[FEATURE REQUEST] Add an option to disable minified JSON graph serialization

Forums 💬 NodeCanvas 🗨️ General Discussion [FEATURE REQUEST] Add an option to disable minified JSON graph serialization

Viewing 12 posts - 1 through 12 (of 12 total)
  • Author
    Posts
  • #13094
    kiamvdd
    Participant

      When working with version control, the minified json used in graph assets can be a bit of a pain to work with, as any small change to the graph result in huge diffs that can be annoying to sift through. Using pretty print instead isn’t a perfect solution, as formatting is kind of ruined once written to the asset (and I’m not sure we have any control over this?), but I still believe it’s a more readable solution for version control.

      I think it would be a good middle ground to keep the default minified, but allow us the option to enable pretty printed serialization. This would also have to be a project based setting that can be picked up by version control so the entire team uses the same setting for serialization.

      #13105
      Gavalakis
      Keymaster

        Hello there,

        This is something I’ve been struggling with in the past, but unfortunately serializing the json in a clean pretty format is not possible. A serialized pretty json string field in a Unity Asset, actually ends up looking worst than a minified one and not very helpful for diff purposes.  🙁 Here is an example of how it looks:

        [attachment file=”SerializedPrettyJSON.png”]

        #13104
        danielk
        Participant

          This is a pretty big issue for any project working with VCS – would it be possible to write out the serialized json as a companion file and load it in at runtime? Then maybe bake everything together at build time?

          #13103
          Gavalakis
          Keymaster

            Hello there.

            There is already a similar feature suggestion to automatically serialize the graph to an external pretty json file alongside the graph and load from that file in runtime. This is something that I will implement soon. With that said, there is already the possibility of manually exporting the graph serialization to a pretty json file and importing from json in runtime, but of course the goal above it to do this automatically.

            Is this described feature above what you were suggesting as well?

            #13102
            danielk
            Participant

              Wait, so if I manually export the graph – it will load it from the file in runtime going forward?

              I’m not seeing that functionality, but was hoping you could point me in the direction of the serialization so that I could hack a solution. We’re getting multiple daily conflicts and its just not an acceptable workflow

              #13101
              danielk
              Participant

                Here is a first start on reading/writing to disk. This is only editor atm, need to add build logic to bake it BACK in. This is a git patch. I name the resulting json files with a preceding “.” to ensure Unity ignores them. NOTE: this is super hacky. My recommendation would be to serialize into YAML. Mixing data formats is generally going to result in an inelegant solution

                For Graph.cs

                And for GraphOwner.cs (to also generate for bound prefabs):

                #13100
                danielk
                Participant

                  And here is a git patch to resolve the viewport changes – way too many updates due to this:

                  #13099
                  Gavalakis
                  Keymaster

                    Hello again.

                    Thank you. I will look at the changes even though I have already worked on the external file serialization/deserialization 🙂

                    Regarding your last post changes, do you mean about serializing the translation and zoom factor and marking the asset as dirty?

                    Thank you.

                    #13098
                    danielk
                    Participant

                      Hey!

                      This is more of a hack for users who need a solution. Less of a suggestion for you. The second git patch is to disable serializing the translation. Way too many updates for just a view change.

                      Ultimately, though, it looks like node canvas might not meet the needs for our use case. The merge conflicts are just not viable.

                      #13097
                      ciyapa official
                      Participant

                        The automatic serialisation of the graph to an external nice json file alongside the graph and runtime loading from that file is already a suggested feature. This is something I’ll start doing soon.

                        #13096
                        ramonmiles
                        Participant

                          This is a big issue for any project working with VCS – would it be possible to write out the HCMI  as a companion file and load it in at runtime? Then maybe bake everything together at build time?

                          #13095
                          danielk
                          Participant

                            [quote quote=16301]Hello again.

                            Thank you. I will look at the changes even though I have already worked on the external file serialization/deserialization 🙂

                            Regarding your last post changes, do you mean about serializing the translation and zoom factor and marking the asset as dirty?

                            Thank you.

                            [/quote]

                            This would be for anyone else who needs a workaround until you have your external changes live (like me and the OP).

                            And yes – that is nuking the translation + zoom serializing changes. I saw some questions asking to disable that, and I did it for myself. Thought I would share the patch as well.

                            Its just a shame that Unity doesn’t support chomping indicator |, or this would be so much easier

                          Viewing 12 posts - 1 through 12 (of 12 total)
                          • You must be logged in to reply to this topic.