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

Supporting more and improved data serialization options #10

Closed
willnationsdev opened this issue Oct 9, 2020 · 10 comments
Closed

Supporting more and improved data serialization options #10

willnationsdev opened this issue Oct 9, 2020 · 10 comments

Comments

@willnationsdev
Copy link
Contributor

From what I can tell, you used to use your character resources for data, but you've since migrated to using JSON for everything.

However, there are distinct pros and cons to each data format.

  • Resources:

    • visual editing with drag/drop support
    • type-safe use of properties
    • support for methods, signals, constants
    • automatic data-sharing at global scale (separately loaded resources with the same path share the same instance; this is not true of Dictionaries which will produce duplicate when loading the same file multiple times).
  • JSON:

    • doesn't have any of the 3.2.x frustrations with exporting user-defined resources, namely because it's all done via code anyway.
    • can easily edit data in any text editor with a commonly understandable format that does not break as a result of Godot API changes.
    • easier to export to or import from alternative editor software.

It'd be better if multiple serialization options could be supported simultaneously, presenting users with options to suit their preference/scenario.

I can also see that you manually parse and put together the data in the editor_view.gd script which performs many other tasks as well. Might I suggest the following changes?

  • Use the Strategy pattern to define serialization strategies (or just use an enum inside some Saver class). Allow JSON and Resource as options. Make the selection configurable from the UI.
  • Switch to exclusively using Resources for in-editor work, rather than Dictionaries.
  • Switch to using inst2dict and dict2inst to automatically convert resources to and from JSON formats.
  • Write a separate class that exclusively handles the data for the app, e.g. a DialogicDB class. Have it handle your "iterating over directories" work, building the object instances, and storing them in Arrays/Dictionaries/whatever for use elsewhere. Then, have your editor_view.gd just create an instance of the DialogicDB class and initialize it. This will help clean up the editor_view.gd script.

Also, long-term, because editing arrays of resources in the Inspector kinda sucks, we could consider implementing a spreadsheet-like editor in the bottom panel for editing multiples of these resources at once (with each row being a different instance), at which point, the entire collection of resources could be saved into a single dialog_db.tres file. Give people the option of saving each resource out to a separate file in a directory, but also give the option to just have it all saved to a single resource file that stores an array of the other resources internally. There's actually already a proposal I wrote about it at godotengine/godot-proposals#13. Not a requirement of the above changes though.

@coppolaemilio
Copy link
Collaborator

It was impossible to effectively modify resources from a tool made using gdscript. I tried many things but it was really hard and even when managed to save updated resources they wouldn't update properly. Going with JSON was the easiest solution and I like the simplicity of it.
Adding resources as options would make much harder the entire thing and I really want to "KISS" (keep it stupid simple). I might split the code and separate the part that deals with files and the part that deals with the editor, but for now I think it works faster for me to have it all in one place.
Reading the proposal seems like implementing what you are saying will just be a lot of work to end up in the same place we are right now. The design idea behind this plugin is that you add it and don't think about it later, but if we start adding options for resources, files, where to save, single resource, many resoureces, editing in files, editing in the inspector, etc.. will just make the plugin feel less "magical".

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Oct 9, 2020

if we start adding options for resources, files, where to save, single resource, many resoureces, editing in files, editing in the inspector, etc.. will just make the plugin feel less "magical".

I mean, if your goal is really to make it a "magical" and opinionated editing experience that hides the details from users, then I would still recommend finding a way to use resources if possible. All of the other suggestions I made were just assuming you'd want a less opinionated API.

You could just use a single dialogic_db.tres file to house all of the data related to the plugin for any given project. That hides the details far more than any collection of directories filled with foreboding <key>-<timestamp_id>.json file names would.

Something like this:

extends Resource
class_name DialogicDB
export var characters := []
export var character_keys := {}
export var dialogs := []
export var dialog_keys := {}
func add_character(p_key: String, p_char: Resource) -> void:
    if character_keys.has(p_key):
        characters[character_keys[p_key]] = p_char
    else:
        character_keys[p_key] = characters.size()
        characters.append(p_char)

# usage
extends Node
func _ready():
    var db = DialogicDB.new()
    var char_res = CharacterResource.new()
    char_res.name = "Bob"
    char_res.image = load("icon.png")
    db.add_character(char_res.name, char_res)
    ResourceSaver.save("dialogic_db.tres", db)

Reading the proposal seems like implementing what you are saying will just be a lot of work to end up in the same place we are right now.

Using resources can lead to more maintainable code as you have a guarantee of the structure of each record, the data types associated with them, autocompletion, etc. You can define setters for properties to abstract how the data is internally manipulated. You can define methods for the class. You can define signals that allow your "records" to signal other objects to react to its mutations. There are so many possibilities that Dictionaries just don't support. So, it is far from being "in the same place we are right now."

Adding resources as options would make much harder the entire thing

In what way? The biggest drawback of resources is not being able to export them to the Inspector out-of-the-box, but you are creating a dedicated editor for them directly in your plugin, so that isn't even a concern. There aren't a lot of distinct advantages with Dictionaries I can think of except some insignificant memory saving within a given local scope.

It was impossible to effectively modify resources from a tool made using gdscript. I tried many things but it was really hard and even when managed to save updated resources they wouldn't update properly.

I'm actually very curious about which issues you encountered here. I'd be interested in figuring out how to resolve these issues. JSON certain works and is simplistic in nature, but the simplicity also results in limited features and less capacity to abstract and organize your codebase safely. I'd even be willing to try creating a working resource-based approach in a forked branch for you to test and play with.


Regardless, if you're adamant that you want to stick to JSON, and don't want to revisit the possibility of resources, then no harm done. I'll just drop the issue. Don't want to divide development with some competing repository or anything like that.

Edit: Hmmm, I wonder if it's serializing a list of user-defined Resource types that resulted in trouble for you. I haven't specifically tried that before, so I'm not sure how Godot 3.x handles the serialization of the data, or whether it can properly deserialize. Will have to experiment with that...

Edit 2: So far, I've encountered two things that are potentially problematic in the workflow, but both of which I have found can be resolved:

  1. Exporting a resource with a Resource Array property can be a problem if you re-save the scene, triggering a re-save of the initial scene state of the data. Saving the scene triggers saving the resources which, if tied to the same file, can lead to all runtime-updated data being cleared out at editor-time. Solution: don't mix runtime and editor-time resource file paths and/or don't export a Resource with such a property.
  2. Due to a Godot 3.x bug, exporting an Array results in a shared Array instance across all object instances. Solution: Don't export array; use a tool resource with reflection methods to simulate faux properties containing all values in the internal array.

I'll report on more issues/solutions as I move along with testing the idea.

Edit 3: FYI, I cleaned up and expanded upon the implementation of my serializable resource map code and ended up with this ArrayMap implementation. Just in case you wanted to look at it. I've been experimenting with having a DialogicDB resource script that has these ArrayMaps as properties, and it has been very successful in safely storing instances of DialogCharacterResource. :-)

@AnidemDex
Copy link
Contributor

Hey @willnationsdev is this what you meant?

This is a custom resource (TimelineResource) that has an events array. Every event is a EventResource based and every event can had its own behaviour.
They can be saved as a resource file inside the resource tree (as like any other resource).


image

image

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Apr 5, 2021

Yeah, although, if it had been me, I'd likely try to find a way to implement everything as resources. So, you'd have each character be a resource. They would have hash IDs for lookup in some sort of CharacterStore resource. That would be independent of a TimelineStore with Timeline resources. And then all the various Stores would be in a master DialogicStore resource that holds everything.

Now, I remember I tried implementing a CharacterStore in my fork at one point, and I ran into a bug where Resource elements in a Dictionary wouldn't persist properly. Ended up simulating them all as separate properties instead, and then I took that concept, abstracted it, and created a class in Godot Next for it (something like HashArray?). Never got around to testing it further or merging the character implementation into Dialogic. And to my understanding, it was a bug that will be fixed in a future Godot version anyway (already in 4.0, not sure if already backported).

But yes, I did mean having actual Resource types to represent the data types used by Dialogic. Even in a "worst case scenario" of only using JSON files, you could probably still write script classes that extend ResourceFormatLoader in order to define your own custom format type that matches on .character.json or .timeline.json or something to have those files auto-recognized as specific Resource instances. There are many possible solutions.

@AnidemDex
Copy link
Contributor

I'd likely try to find a way to implement everything as resources. So, you'd have each character be a resource.

I'm trying to make everything like a resource as well.
image

Thank you for answering this very old problem, I will take your advice and see what happens if I try to make a PR (since the dialog_node is becoming too complex)

@AnidemDex
Copy link
Contributor

AnidemDex commented Apr 7, 2021

@willnationsdev I tried to save the DB (using ResourceSaver.save) after modifying its properties while testing in game (because i'm too lazy to reload the plugin). The resource file is not saved.

There's no problem if i do it with the editor (as far i know based on godotengine/godot#6603). I was just wondring if you know a workaround (at least until godotengine/godot#30302 * get closed)

@willnationsdev
Copy link
Contributor Author

willnationsdev commented Apr 7, 2021

@AnidemDex Can't exactly suggest a workaround until we know what the actual problem is. You'd need to start with a basic resource save and move up in complexity until you reach the same level of complexity as your DB resource, just to ascertain what exact conditions prevent proper saving. I suspect it has something to do with recursive saving of custom resources within an exported Array property on another resource script, but I'd need to know what actual logic your DB script has in order to better diagnose the problem. If it is the above case, you might have to try using the ArrayMap script I wrote for Godot Next (or something else which similarly generates faux properties).

Also, issue 6603 appears to already be closed, so I'm not sure if you meant to refer to a different issue, or...?

@AnidemDex
Copy link
Contributor

Also, issue 6603 appears to already be closed, so I'm not sure if you meant to refer to a different issue, or...?

@willnationsdev My mistake, i was talking about #30302, i edited my previous message.

Don't worry. While saving the database doesn't work in game, the editor can save the database while its excecuted as plugin, as expected.

... but I'd need to know what actual logic your DB script has in order to better diagnose the problem.

I store an array of strings (PoolStringArray) that gives me the path for every resource, instead saving the resource directly. You can see it here

... you might have to try using the ArrayMap script I wrote for Godot Next.

I tried to use it, I swear, and while I was able to save the resources, I didn't know how to expose them in the inspector. I need expose them to know if the editor plugin is doing something without a wall of print_debug calls, at least, while debugging. I will probably need it later, but I have not been able to understand its nature.

@willnationsdev
Copy link
Contributor Author

I'll have to look at your script when I get a chance. As for the ArrayMap, there's a decent chance there are issues with it since I never got to fully test it when I was writing it. XD So it may not necessarily be something you're doing wrong. No worries. If the properties aren't being exposed to the editor, then it's probably because the _get_property_list() method isn't properly adding the PROPERTY_USAGE_EDITOR flag to their usage value. That's what makes properties appear in the Inspector. I don't have time to debug it now though. Sorry. Lots of stuff with work.

@coppolaemilio
Copy link
Collaborator

I'm closing this one since we are now using resources for the new stuff

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

Successfully merging a pull request may close this issue.

3 participants