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

Generate typed equivalents of Json.hx for other languages #238

Closed
deepnight opened this issue Nov 20, 2020 · 26 comments
Closed

Generate typed equivalents of Json.hx for other languages #238

deepnight opened this issue Nov 20, 2020 · 26 comments
Assignees
Labels
🧪 Experimental This issue might or might be not addressed. 🙋‍♂️ Help wanted Extra attention is needed
Milestone

Comments

@deepnight
Copy link
Owner

QuickType:
https://github.com/quicktype/quicktype

Json schema + quickType:
https://github.com/elnabo/json2object/#using-the-jsonschema-writer

@deepnight deepnight added the 🧪 Experimental This issue might or might be not addressed. label Nov 20, 2020
@deepnight deepnight added this to the 1.0.0 milestone Nov 20, 2020
@deepnight deepnight self-assigned this Nov 20, 2020
@zicklag
Copy link
Contributor

zicklag commented Jan 3, 2021

Hey there! I am kind of needing this right now so that I can use this in my Rust project. Could you point me to the code that generates the current JSON doc so that I could take a shot at generating JSON schema with it?

Edit: Nevermind, I found it. I'll open a PR if I do anything useful. 🙂

@deepnight
Copy link
Owner Author

Started some experiments on 0.6.3 branch. Here are some files that needs some feedback to verify the quality of QuickType output:

python.zip
rust.zip
csharp.zip

@deepnight deepnight added the 🙋‍♂️ Help wanted Extra attention is needed label Jan 4, 2021
@deepnight deepnight modified the milestones: 1.0.0, 0.7.0 Jan 4, 2021
@Mudloop
Copy link

Mudloop commented Jan 4, 2021

The generated C# code has this :
public static object FromJson(string json) => JsonConvert.DeserializeObject<object>(json, ldtk.Converter.Settings);
but this should be :
public static LdtkJsonClass FromJson(string json) => JsonConvert.DeserializeObject<LdtkJsonClass>(json, ldtk.Converter.Settings);

So it's returning a plain object instead of the correct typed object.

When testing on https://app.quicktype.io/ with some dummy JSON, it does add the correct return types, so something seems to be going wrong when you generate the file.

Additional note : For Unity, it would be easier if the file extension was .json instead of .ldtk (or maybe .ldtk.json). Might be too late to change that now, but it would also have the benefit of enabling syntax highlighting in code editors by default.

@zicklag
Copy link
Contributor

zicklag commented Jan 4, 2021

For the Rust version, I haven't tried the verbatim generated code form Quicktype, but I did have to make some of the non-optional types optional, such as array_max_length and about 5 others or so I think. They were all integers that were not marked as optional in the schema, but were actually set to null in LDtk sample map for testing feature completeness of loaders.

Additional note : For Unity, it would be easier if the file extension was .json instead of .ldtk (or maybe .ldtk.json). Might be too late to change that now, but it would also have the benefit of enabling syntax highlighting in code editors by default.

Counter to that point, there certain things like setting your desktop to open LDtk files, where it is better to have the ldtk extension so that you don't just open it as a JSON but with the editor, and also for the custom asset loader I made for the Bevy game engine, it knows to load the asset as a map, and not a JSON, specially because it ends in ldtk. Granted that would still work if you did .ldtk.json. 🤔

@Mudloop
Copy link

Mudloop commented Jan 5, 2021

Counter to that point, there certain things like setting your desktop to open LDtk files, where it is better to have the ldtk extension so that you don't just open it as a JSON but with the editor, and also for the custom asset loader I made for the Bevy game engine, it knows to load the asset as a map, and not a JSON, specially because it ends in ldtk. Granted that would still work if you did .ldtk.json. 🤔

I can definitely understand that having a custom extension has its benefits.

I guess having it as an option might be the best route?

@zicklag
Copy link
Contributor

zicklag commented Jan 5, 2021

It's actually already an option! ( But this probably isn't the best thread to continue this discussion on. )

image

@Cammin
Copy link

Cammin commented Jan 5, 2021

I can softly confirm that this successfully deserializes the C# schema. Though have not tested checking any values yet, myself.
The only vital change I made to the json schema was by changing the data type from object to LdtkJsonClass, the same as what MudLoop found:
From this:
image
To this:
image

@deepnight deepnight pinned this issue Jan 5, 2021
@deepnight
Copy link
Owner Author

@Cammin @Mudloop
This problem should be fixed with the new Json Schema (https://ldtk.io/files/JSON_SCHEMA.json)

@deepnight
Copy link
Owner Author

@zicklag

For the Rust version, I haven't tried the verbatim generated code form Quicktype, but I did have to make some of the non-optional types optional, such as array_max_length and about 5 others or so I think. They were all integers that were not marked as optional in the schema, but were actually set to null in LDtk sample map for testing feature completeness of loaders.

These should be marked as optional in latest schema version :)

@estivate
Copy link

estivate commented Jan 7, 2021

Using the latest schema & API sample test from master branch, I can easily get a simple example working in Rust. I don't know if this is what I am supposed to be doing with this, but here's what I did:

  1. Paste the schema in this repo into https://app.quicktype.io/ with the Name set to "LdtkJson" and the type set to JSON Schema, then on the right side set the Language options to Rust, Dense, Public and left derive Debug off.
  2. Copy the resulting code and paste it into a file named "ldtk_json.rs" in a blank rust project.
  3. Change the line at the top of ldtk_json.rs that reads "extern crate serde_derive;" to "use serde::*;"
  4. Add the following to the dependencies section of Cargo.toml:
[dependencies]
serde = {version = "1.0", features = ["derive"]}
serde_json = "1.0"
  1. Change main.rs to read:
use std::fs::File;
mod ldtk_json;

fn main() {
    let file = File::open("assets/test.ldtk").expect("file not found");
    let data: ldtk_json::LdtkJsonClass =
        serde_json::from_reader(file).expect("error while reading");
    println!("default bg_color is {}!", data.bg_color);
}
  1. Create an "assets" folder at the top level of the project and copy the contents of an LDTK JSON file into "assets/test.ldtk". In my test above I used the API test file from LDTK's samples directory.
  2. Run "cargo run"

I think this means that "it works" and the entire JSON file is being parsed successfully. Otherwise I think I'd generate an error reading in the file. Will try and use it in an actual game example next.

@zicklag
Copy link
Contributor

zicklag commented Jan 7, 2021

Yep, that means that it works. There are a few things that bugged me about the generated Rust types, even though they do technically work:

  • Vectors are always Vec<Option<T>> when they really just need to be Vec<T> because you'll never have a null item in an array.
  • All of the types such as Definitions are unnecessarily an Enum that matches on both an actual DefinitionClass and also any other kind of JSON object. I don't know why it does this, but really it should just require everything to match the DefinitionClass and so enforce the static typing and prevent you from having to do an if let or a match every time you need to access a Definition.

In short, those issues will make you write a lot more code to access the values, because it assumes a level of variation that isn't actually going to be in the schema.

I'm pretty sure that is all either because of a fundamental limitation of JSON schema, or because QuickType doesn't generate the best Rust code, or a little of both. I think the JSON schema itself is accurate.

That said, with just a little bit of extra Regex on top of the Quicktype generated schema, we can easily get it much more Rusty, and I have published a work-in-progress Rust crate for LDTK with the manual changes to clean it up a bit: ldtk-rs. I still want to go through and fix a couple things with it: I'll be polishing it as I use it to implement bevy_ldtk plugin.

@estivate
Copy link

estivate commented Jan 7, 2021

Yep, seeing that as I try to replace my ldtk_rust crate with this. Looks like we took similar approaches and are wrestling with the same complications. I was hoping that the schema was not totally fleshed out yet and some of these optional values would clean up over time, but I'm not familiar enough with schemas or QuickType.

I'll keep going with this auto-generated code to learn more, and also check out your crate. Maybe this auto-generated code can be wrapped in a lib to make it more accessible while at the same time easy to keep up to date.

@zicklag
Copy link
Contributor

zicklag commented Jan 7, 2021

@estivate that's funny, I didn't realize when I started my crate that there was already your crate and somebody else also had this one. 😄 Being as I took the bevy_ldtk and ldtk crate names and both of you actually started your crates first, let me know if you would like to collaborate.

On the topic of auto-generation, it is worth noting that we might try generating Rust directly from the DocGenerator.hx, which would probably allow us to get it exactly how we want it, instead of depending on QuickType's interpretation of the JSON Schema. QuickType was just an easy way to instantly get type generation for a much larger collection of languages in one go. It was a reallly simple script to generate the JSON Schema, so it wouldn't be hard to port it to generating Rust instead. I think that's probably our best bet.

Still, to get niceties like converting the flip bits to a nice TileFlip struct would be something that would have to be manually added to extend the generator and cover the special cases. But again, even that wouldn't be that bad.

@deepnight
Copy link
Owner Author

On the topic of auto-generation, it is worth noting that we might try generating Rust directly from the DocGenerator.hx, which would probably allow us to get it exactly how we want it, [...]

Well actually, it's a better approach to rely on the JSON schema, because it's purpose is specifically to describe the file format while being language agnostic. And this one is guaranteed to be always up to date in future updates as it'll be the backbone of importers.

Quicktype is just a "JSON Schema to whatever" converter.

@estivate
Copy link

estivate commented Jan 8, 2021

Before we get too deep into the details, can we first look at the LdtkJson.rs file posted on the site and see if anyone can get it to compile and give access to anything? It may not work well, but I'm having two problems getting it to work at all:

  1. I have to change the first line from extern crate serde_derive; to use serde::*;
  2. I have to put pub in front of all the fields I want to use

I'm curious what @zicklag and other Rust folks think because I may not be using the right techniques. There is a serde_derive crate, but I usually use their other crates with a features = ["derive"] tag. And for the second, I am not aware of a way to access private fields in a module externally, which is how I would assume this .rs file would be designed to be used?

If indeed these issues need to be corrected, we might be able to do so in the generation process, or by commit to the QuickType project. Or if we can't fix I think a quick note to new users would be helpful, especially since it takes less than a minute to fix!

For completeness, I'll also note that the commented instructions at the top also don't work for me. They of course don't cause compile issues, but they slowed me down the first time. If they are misleading we should remove or replace if possible, or at least document for first time users.

@zicklag
Copy link
Contributor

zicklag commented Jan 8, 2021

Well actually, it's a better approach to rely on the JSON schema

True. Then we should just write a JSON Schema → Rust converter that we can put into the build.rs file to automatically generate the Rust code when the crate is built. I think that would probably work well. The biggest thing I was worried about when relying on the JSON schema is the fact that it seemed that the JSON Schema spec didn't have a way to represent non-nullable items in an array. Maybe I missed it, or else it is just safe enough to assume that elements in arrays are non-nullable. I'd have to double-check that.


@estivate to cover your questions:

I have to change the first line from extern crate serde_derive; to use serde::*;

Yes, that should be changed, apparently quicktype uses the older rust import strategy. It should be use serde::{Deserialize, Serialize};.

I have to put pub in front of all the fields I want to use

There is a Rust specific option to pass to the quicktype CLI or some option in the web interface that adds pub to all the fields.

There is a serde_derive crate, but I usually use their other crates with a features = ["derive"] tag. And for the second, I am not aware of a way to access private fields in a module externally, which is how I would assume this .rs file would be designed to be used?

Yep, all your guesses are correct. Those fields should be public and you should be able to use serde = { version = "1", features = ["derive"] } in your Cargo.toml.

I think a PR to Quicktype would be reasonable to fix crate imports and comments at the top of the file, and otherwise we just need to pass in the --visibility public to the command used to generate the file.

@estivate
Copy link

I've converted ldtk_rust to use the QuickType generated code and updated it to work with the 0.6.3-preview JSON Schema, including combined or separate level files (all in the qtype branch here). I added an example loading a single external level at a time. The Bevy example needs some work but it does compile, so I think the QuickType auto-generated code is going to work well.

Could we add defs to the list of required properties in the top level LdtkJsonRoot object in the JSON Schema? Every .ldtk file I have seen has a defs object, even if some of the children are empty arrays. The docs on the site also don't indicate that it could be null, but perhaps I haven't seen a case where it would need to be null.

@zicklag
Copy link
Contributor

zicklag commented Jan 19, 2021

@estivate did you have to modify the types generated from QuickType? It looks like those are different than the ones I got when I tried QuickType out earlier. It doesn't have those annoying [Something]Class Enums anymore.

@estivate
Copy link

@zicklag I used the JSON Schema from the 0.6.3-preview2 branch exactly as generated (except for setting visibility public and updating the serde import). As far as I can tell the main differences using the auto generated code:

  1. I can now support 100% of the JSON data "for free" :)
  2. When LDtk releases updates to the schema it should take about five minutes to support it.
  3. There are a lot more optional fields than I had before. In most cases these were bugs waiting to happen in my hand crafted version. In some cases I expect it's a missing "required" in the schema. That's why I was asking about the "defs" field above.
  4. There are a few places where I would have defined a struct but the auto generated code made a HashMap over a serde Value type. It still works but it's a bit harder for the end user to consume. Examples include LayerInstance.intGrid and EntityInstance.__tile. Technically the auto generated code is correct (or at least less brittle) but I will likely try to wrap those somehow with some convenience methods or something just to make my life easier.

It's almost to the point where we don't really need our libraries any more. I only have a lib.rs file and it's tiny. There may be an ongoing need to provide some "quality of life" wrappers around the QuickType loader, but you could likely accomplish that with documentation or a blog post instead of an actual library.

@zicklag
Copy link
Contributor

zicklag commented Jan 19, 2021

@estivate very cool. 🙂 Once I get some time for this, I might try to make a Rust version of the QuickType Rust generator that it could be included in the build.rs. It would also allow you to create "hooks" that allow you to modify the generated types so that we could add quality of life improvements like making the X/Y tile flip a struct instead of a bitflag, etc. Then you completely automate the generation of the types with Pure Rust and maybe even optionally grab the latest version of the JSON schema off the website in the build.rs using a feature flag or something.

But anyway, it looks like you've got it going pretty well with the QuickType one so anything else is just a bonus. 👍

@deepnight
Copy link
Owner Author

deepnight commented Jan 20, 2021

Latest JSON schema is now fully typed, with the following exceptions:

  • various fields marked as "internal editor data", not meant to be used in games
  • the __value of a FieldInstance, as this value type depends on the field definition type.

See this issue for a downloadable ZIP: #319

@estivate
Copy link

Thanks @deepnight ! The Schema is looking really good and this will makes it even easier for Rust consumers.

@deepnight deepnight removed this from the 0.7.0 milestone Jan 21, 2021
@deepnight deepnight added this to the 0.8.0 milestone Jan 21, 2021
@zicklag
Copy link
Contributor

zicklag commented Jan 22, 2021

I just published a new alpha release of my ldtk Rust crate which is now 100% automatically generated using a custom JSON schema → Rust generator that is built into the build process for the crate. You can also optionally have the crate automatically download the latest schema from GitHub during the build so there is absolutely no manual process necessary to get up-to-date bindings.

There are a couple of modifications automatically applied to the generated code such as adding a TileFlip struct to replace the bitset used in the tile map and renaming a couple of fields for a better user experience. We can add more improvements if necessary.

You can see the generated API docs here.

@deepnight
Copy link
Owner Author

Feel free to re-open is more info are needed 👍

@zicklag
Copy link
Contributor

zicklag commented Feb 26, 2021

Sounds good. 👍

Would it be possible to have my LDtk crate added as an alternative Rust implementation to the website? Also, if you do, could you put it as "by Katharos Technology" since it's a project under my organization.

@deepnight
Copy link
Owner Author

deepnight commented Feb 28, 2021

@zicklag Sure, feel free to add your info here: #273

@deepnight deepnight unpinned this issue Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧪 Experimental This issue might or might be not addressed. 🙋‍♂️ Help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants