Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented basic .wdz file opening and verified that JSON files can be read in chiventure #800

Merged
merged 19 commits into from
Jun 8, 2020

Conversation

namanhd
Copy link
Contributor

@namanhd namanhd commented Jun 8, 2020

(a reopen of #798 with the correct branch this time)

The branch implements some basic wdz file loading and prints out the contents of the JSON files contained inside to the terminal behind chiventure's main UI. This serves as both a demo for the team, a first introduction of libzip and json-c dependencies to the chiventure codebase, and a proof of concept that the .wdz file loading functionality is on the right track.

This does not, however, convert the parsed JSON objects into game objects, which should be implemented later. (in branch wdl/parsing-wdz-namanh).

Addresses #529 but does not fulfill it completely. We will make a separate backlog issue for final conversion of json objects into game objects.

@namanhd namanhd added the x/wdl label Jun 8, 2020
@namanhd namanhd added this to the 2020/Sprint 4 milestone Jun 8, 2020
@namanhd namanhd requested review from MaxineK36 and dtbukowski June 8, 2020 19:01
@namanhd namanhd self-assigned this Jun 8, 2020
@@ -6,7 +6,6 @@
#define INCLUDE_ATTRIBUTES_H
#define MAXLEN_ID 60 // ID strings for objects

typedef struct obj object_t; // forward declaration so attribute_t can use
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this change as attributes.c doesn't use object_ts

Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the code looks good, just a couple of style things

@@ -17,6 +19,21 @@
*/
game_t *load_wdl(char *path_to_yaml)
{

// WDZ loading monkeypatch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a block comment

}
else if (json_object_is_type(j_value, json_type_object))
{
// The only file with an object-type value as top-level
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hsould be a block comment


#include "wdl/load_wdz_internal.h"

// maximum buffer size for json file, in bytes. This is currently set to 2 MiB.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block comment

return NULL;
}

// Don't try to JSON-parse files that don't have a .json extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block comment

return NULL;
}

// buf now contains the raw JSON. Use parser to convert into json obj.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block comment


// buf now contains the raw JSON. Use parser to convert into json obj.
// print out behind the main chiventure executable for debug and demo.
printf("%s\n", (char *)buf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are these printing directly to stdout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was for debug purposes and to not disrupt the UI of chiventure when it loads up. Is there a better place for me to output this? Since it will also be how we'll showcase that our code loads the zip file and reads the json files properly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok; that's fine

if (j_obj) // This is NULL if the path doesn't end in .json
{
load_game_objects_from_json_object(obj_store, j_obj);
// increment the current number of json files.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment should go on the same line as count++;

Copy link
Contributor

@dtbukowski dtbukowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good besides the fact that some comments at certain areas are using single line format when they use multiple lines but they explain what the print statements/code does so the only thing that would change is the format of these comments.

@MaxineK36 MaxineK36 self-requested a review June 8, 2020 19:33
Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@MaxineK36 MaxineK36 self-requested a review June 8, 2020 19:38
Copy link
Contributor

@MaxineK36 MaxineK36 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nvm, all is working

@MaxineK36 MaxineK36 merged commit 13b22f9 into dev Jun 8, 2020
@namanhd
Copy link
Contributor Author

namanhd commented Jun 8, 2020

Successfully merged some basic .wdz zip opening and JSON reading functionality into chiventure. This can be invoked by running the main chiventure executable, passing in a file that ends in .wdz with the right directory structure, and quitting out of the main chiventure UI (with Ctrl+D); the contents of all the json files in the .wdz archive should have been printed out onto stdout.

This PR introduces libzip and json-c dependencies into the chiventure codebase, and also serves as a foundation for the more complete implementation of object imports later on.

@MaxineK36
Copy link
Contributor

Issue Score: ✔️++

Comments:
Great work! Thanks for linking all relevant issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants