Store resources as text for better colaborative workflows

[TitanNano]

We are currently struggling with the fact that all engine resources (scenes, tile maps, etc.) are stored in binary form. While this is great for performance, it makes it quite difficult to handle these files in git and do code reviews on them. It's not possible to resolve merge conflicts in binary files, and the changes inside the resources are opaque. It's also quite challenging to notice accidental changes.

Godot, for example, offers two file formats for most resources. A text-based format and a binary-based one. This helps greatly with handling the files in git and makes it simple to review changes.

I suggest adding the option to store every resource in text form in a project and only convert them to their binary form during export.

[mrDIMAS]

Hi! This is planned, I even looked in this direction few weeks ago, but unfortunately got distracted by some other tasks. I'm currently working on adding UUIDs for resources and after this (maybe in the beginning of the next week) I'll start working on a text format for the engine assets.

[mrDIMAS]

Resource system refactoring took a bit more time than I expected. I'll try to start working on this issue on this week.

[mrDIMAS]

So, I've added ASCII format for the engine assets, but it still needs integration in the asset pipeline. I think the best way is to write all the assets in ASCII mode when saving them from the editor, but re-save them in binary format for production builds when exporting a project. Anyways, it will take some time (a day or so) to this integration properly.

[TitanNano]

I think the best way is to write all the assets in ASCII mode when saving them from the editor, but re-save them in binary format for production builds when exporting a project.

Yes this sounds like a great approach. 👍

[mrDIMAS]

I've submitted a change that forces the engine to save assets in the new text format. The next part is to add text -> binary conversion when exporting the project.

I'd advise you to check if it suits your needs by making a separate branch where you can test how the changes merge and if the engine works as it should. Please report any issues here or in the issue tracker. As usual - if you're using latest changes from the repo, you're doing it at your own risk.

[mrDIMAS]

I'll accept anything that will be good for collaborative work, that produces as less merge conflicts as possible. It is hard for me to understand whether current implementation is good for this or not, or maybe yours is better. Simply because I'm working alone on my game and there's no such thing as merge conflict when you're working alone.

[TitanNano]

I tested both implementations much more today.

Unfortunately, neither of them work very well.

My Implementation

I don't think nesting, or not-nesting, changes much about the merge ability. Where my solution falls apart is the fact that the VisitorNode index is not stable at all. So making small changes to a scene already causes indexes to change everywhere.

The current master implementation

Making small changes like moving a node inside the tree can cause changes across the entire file.

---

After looking more closely into the behavior of the serialization, I understand now that basically the entire Rust type structure is being dumped into a file. This makes for a very straightforward implementation, but sadly creates a very generic representation that doesn't lend itself well to being used in Git.

We get good merge ability by having small, unique changes across a small number of lines.

i.e.:

[node type=__ROOT__ id=123]
some_field = true

[node type=RigidBody id=a43a parent=123]
enabled=true
visible=true
name=Player

[node type=RigidBody id=bb63 parent=123]
enabled=true
visible=false
name=Mob

has more unique information on each line than:

{1:
    __ROOT__
    [1:
        some_field<bool:true>
    ]
    {2:
        RigidBody
        [3:
            enabled<boo:true>
            visible<boo:true>
            name<Data:UGxheWVy>
        ]
        {0:
        }
        RigidBody
        [3:
            enabled<boo:true>
            visible<boo:false>
            name<Data:TW9i>
        ]
        {0:
        }
     }
}

Here unique information is interspersed with very generic information. Diffs are also mostly line-based, so almost all diffing tools will produce very unhelpful diffs.

---

I'm also wondering if the inclusion of the exact structure of the rust types will not be detrimental in the long run:

__ROOT__
[0:
]
{1:
	Scene
	[0:
	]
	{4:
		Graph
		[0:
		]
		{6:
			Root
			[2:
				Index<u32:0>
				Generation<u32:1>
			]
			{0:
			}
			Pool
			[0:
			]
			{2:

The detail that a scene contains a Graph which contains a Pool does not add more information to the store data.

---

Apologies for dumping all this here. I hope this is helpful.

[mrDIMAS]

Good points. I initially thought that more lines will help with merge, but turns out that I was wrong. I think it is worth trying to print all attributes (in rectangular brackets []) in a single line. Then remove attribute ([N:) and node ({N:) counters so this number won't produce conflicts and add yet another possibility of incorrect merge. Anyways, thanks for the info, I'll experiment with this more and will try to improve this situation.

[mrDIMAS]

Ok, turns out that attribute and node counters does not produce merge conflicts, because they're 99.9% of time static.

I've also changed the text format to put attributes one the same line and did some other tweaks and now this is how adding a node and moving some other node looks like in the diff:

@TitanNano could you please check the latest changed again and confirm (or not) that it has become better?

[TitanNano]

My tests today showed that the latest changes work much better.

A few concerns that remain:

1. the node counters are not static in cases like the node pool and the node child handles. There, the counter will increase with each node that is added. If two changes add multiple children this will lead to conflicts and in the case of the node pool the conflict will be at the top of the file while the actual node data is maybe somewhere at the bottom. A worse case occurs when two changes add the same number of children. In this case, there will not be any conflict, but after merging both changes the data will be inconsistent because there are more entries than what is indicated by the counter.
2. Adding a new node into the tree causes all Node Handles to shift around, which can cause many changes and conflics. Take this simple example:

and change it to:

This causes rippling effects throughout the file:

The amount of changes increases with each node that is already part of the tree. The first two changes are purely side effects from adding a new node to the pool.

(Maybe the instance ID could be used to identify nodes in the serialized scene graph instead of the node handles?)

4. something not entirely related to the text format. There is this Data[1:Id<u64:13117957776>] or State[1:Id<u64:105553165091952>] field that changes every time I restart the editor. This causes numerous changes in the file which will cause a lot of conflicts.
5. Similar to 2. there is [1:Flags<u8:3>] which comes from the InheritableVariable type. From what I found, this field is insignificant for the serialized data, but on first write to disk the Flags are u8:1 and when the file is loaded and saved again the Flags become u8:3. I looked at this, and it appears that the actual flags are always inferred from the presence / absence of the value inside the InheritedVariable type and don't need to be stored on disk.

[mrDIMAS]

1. So the counters should be removed, noted.
2. This happens because the editor implicitly adds a bunch of scene nodes to the scene you loads. Not sure how to fix this yet.
3. This can easily be fixed by using a counter instead of using pointer address as an id (as it is now).
4. This is tricky, I tried to remove the flags entirely and it resulted in derived assets corruption. Maybe it's worth saving only required flags like modified and not need_sync.

Anyways, thanks for the info. I'll continue improving this.

[b-guild]

Here are some suggestions for the text format.

• Unify the list of fields and the list of children into a single list of fields and children, in the order in which the values were visited. As things are now, a call to x.visit(...) could create a field for x or a child for x. The interface conceals the representation of x from the user, so it would make sense for the internal representation to handle the two situations similarly. So I suggest that a child be a kind of field, such as: FieldKind::Child(Handle<VisitorNode>). This would mean that the name of a node is stored in its parent instead of in the node itself, but it should be possible to conceal this change from users.

• Fields and children do not always need names, especially in cases like Vec where its children are given names like "Item2" as a substitute for being able to access the children by number. To allow for visiting by number as well as by name, I suggest an enum like this:

  enum VisitorIndex<'a> {
    Name(&'a str),
    Number(usize),
  }

  Along with appropriate From implementations for &str and usize, and then we can make Visitor::enter_region to be generic like this: fn enter_region<I: Into<VisitorIndex>>(&mut self, name: I) -> VisitResult. Unfortunately we would also have to change the signature of Visit::visit to fn visit<I: Into<VisitorIndex>>(&mut self, name: I, visitor: &mut Visitor) -> VisitResult;.

  This change to the Visit trait may require changes in many places, but these changes should only be in the signatures of the methods: the bodies of the methods should not need to change at all. Even the change to the Visit derive macro should be trivial.

• While we should still accept the current format of "name[fields]{children}", we can begin outputting and accepting "name{values}" where the values are a mix of fields and children. We can determine whether a node should be written onto one line by recursively measuring the size of its values and counting the number of fields in this node and all of its children. If the number of fields is greater than some maximum, then we can write each field and child onto its own line.

• We can make the files less verbose by eliminating redundant type information. str:"blah" would still be clearly a string even without the str:. bool:true could be simplified to just true.

• To limit the depth of indenting, we can automatically generate unique names for nodes that are deep within the tree. For example "Base26" might be the 26th generated name for a node named "Base". Then we can replace that node in the file with its name. For example, instead of Base[...]{...} we could have Base:Base26;. At the end of the file, after the full tree has been written, we can go through each generated name and write the corresponding node, as in Base26 = [...]{...}.
  In order to decide whether a node should be replaced by a name, we can have a preferred indentation and a maximum indentation. Once a node is at the preferred indentation level, we can recursively check its children to see if writing this node will cause it to exceed the maximum indentation. If it would exceed the maximum indentation, then we replace the node with a newly generated name and delay writing the node until the end of the file.
  When we are reading in a Visitor we can use a HashMap to keep a record of each generated name we encounter and the position where we encountered it, and then when we get to the end of the file and find the name's value, we can retroactively insert that value in the appropriate place in the tree.

• We can have a policy of minimizing unnecessary regions. For example, we do not need to create a region for Option. If the value is Some(t) then we can simply t.visit(name,visitor). If the value is None then we could simply not visit anything and assume that a missing child/field is None when reading. This alone should greatly decrease the number of regions in most visitors. Alternatively, we might have a special FieldKind::None to represent Option::None and write it as "none". This could be important of we choose to make InheritedVariable flags part of the field.

• InheritedVariable is very frequently used and so it is the source of many regions. Because it is so frequent, we may consider adding flags to every field to give InheritedVariable a place to store the flags without needing to create a region.

• Pool nodes should be written with a special format that accounts for their special nature. Each record in a pool should be written with its handle followed by its value. So instead of Item4[Generation<u32:1>]{Payload[...]{value}} we can write (4:1){value}.

[mrDIMAS]

Issues 4 and 5 should be fixed by now.

[TitanNano]

yes I can confirm these issues appear to be solved. 👍