FlaShG / GitMerge-for-Unity

Unity plugin for merging sceneand prefabs files when using git.
GNU General Public License v2.0
197 stars 26 forks source link

Think about merging GameObject hierarchies #7

Open FlaShG opened 9 years ago

FlaShG commented 9 years ago

Also: Merging a GameObject that has been deleted on one of the branches. "A added a Component, B deleted the whole GameObject."

GameObjects probably need to be displayed in a hierarchy. Child objects probably should not be displayed when the parent is deleted in the current merge state.

I guess that reverting the delete GameObject MergeAction will not recreate deleted child objects right now.

FlaShG commented 9 years ago

"A added a Component, B deleted the whole GameObject."

FlaShG commented 9 years ago

Maybe displaying Transform.parent is enough?

randomPoison commented 9 years ago

Currently are there any technical limitations on preserving parent/child relationships while doing the merge? When I tried GitMerge it completely undid my scene hierarchy, which meant I couldn't use the results of the merge. If possible I'd like to help get this supported so I can start using GitMerge for my project.

FlaShG commented 9 years ago

Since the hierarchy consist only of the property Transform.parent. Since that property is serialized like every other property, the tool should not break any hierarchies. However, I only tested that on flat hierarchies yet...

randomPoison commented 9 years ago

So I put together a simple test to find out in what cases the parent/child relationship doesn't get properly merged. In a scene with a root object with one parent A and a child B, I create a branch where a added a child C and then removed child B on the original branch (so the first branch has just the root A and the second branch has A with a child C). When I tried merging them I wound up with both B and C in the scene, but neither of them were childed to A. From what I can tell, children that haven't moved get merged correctly.

I also tried this with trying to merge hidden properties (as in #15). As it turns out, the parent/child relationship is done as a list of children in the parent, rather than having the child's transform having a link back to the parent:

An example from a different merge.

To properly preserve the hierarchy special effort has to be made to merge the two arrays and then re-connect the parent to the child. As it stands, even when the children list is shown in the merge, you can't use their version because unity gives the error "Transform child has another parent". By the looks of it you can't set an object's children directly, you have to go through the child's transform.parent.

Also, this is something that will benefit from #41. It's really easy to tell what the correct merge is when an object has just been added or removed, but without three-way merging it's impossible to tell what that correct merge is.

randomPoison commented 9 years ago

The hard part of getting Transform.parent exposed in the merge view is that, when serialized, the parent holds the list of of children in its "m_Children" property. That means we need a special case when comparing the components of an object to check if the component is a transform and, if it is, compare the parent between versions. From there it only gets more difficult since changing the Transform.parent value in the child means actually changing the value of two other objects: the old parent and the new parent. I'm working now on putting this all together, the changes in #43 are a step in the right direction.

FlaShG commented 9 years ago

Wait, setting the parent via SerializableProperty does not implicitly update the children? :astonished: //edit: I tried some stuff, here's the result:

FlaShG commented 9 years ago

4d80f88 added a MergeAction for parenting.

I'd like to keep this issue open though, as there is still room to think about hierarchy presentation. Deep hierarchies might be quite a pain to merge by setting parent by parent. Also, the order of GameObjects will be broken. I cannot think of a better solution right now though.

randomPoison commented 9 years ago

How did you test out the parenting merge action? Because in the test scene that I've been using for testing parenting it's not detecting the change in parents at all. Never mind, there was a mistake on my part causing the problem.

Another problem that still needs to be addressed is when an object has been added in "theirs" as a child of another object, or has been removed in "ours" but is still present in "theirs". In both of these cases the object shows up as being at the root level of the hierarchy, even though in both "ours" and "theirs" they were childed to another object. It seems that in cases where an object has been added or removed, some kind of reference patching will be needed to make sure that the merged version of the object is correctly childed to "our" version of their parent.

FlaShG commented 9 years ago

Added this with c3e31c0.

randomPoison commented 9 years ago

Excellent! This was the biggest problem keeping us from using GitMerge on our scenes. I'll start testing this in our game and let you know how it goes.

randomPoison commented 9 years ago

It seems like in the case where:

1) "they" add a new game object to the scene and then 2) move an existing game object to be a child of that new object

GitMerge does not detect that the moved child's parent has changed. I suspect that this happens when the moved child is diffed before the new parent has been added to ObjectDictionaries, so in my case it wasn't detecting that "their" version of the object had a parent (and "our" version of the object also didn't have a parent, hence no difference). If "our" version of the object had a parent I think it would at least detect that there was a difference, but it wouldn't correctly detect "their" version of the parent.

FlaShG commented 9 years ago

0e2e29e adds some behvaiour for this problem. Whenever a parent is set, GitMerge tries to find the version of the object existing in the scene. If it doesn't exist at the time, the user is asked whether or not they want to have the missing GameObject added. The MergeAction responsible for the (non-)existence of the Object will be told to do whatever makes the GameObject exist. I will use the new structures for changed "regular" values, too (see #44).

The thing is still not 100% functional, since noone keeps the user from deleting an object (like the parent) that another MergeAction depends on. Deleting a GameObject that has children involved in the merge currently crashes the tool and leaves behind corrupted scene data.

RDeluxe commented 9 years ago

I have a bug that may be related.

My original scene :

My new Scene :

The result of the merge is :

Edit : I don't know why, seems the last part of my post was lost.

I did the merge correctly, unchecking "auto merge" (I also tried with Auto merging on) and merging each element manually, checking everything. There was a lot of parenting changes, which is good. UIRootCanvas parent was changed correctly, same for EventSystem. Adding the new object "MyScene" automatically added its child "Camp". But nothing about the modifications in UIRootCanvas : just the change of parents. I think that this change overrided the others in some way. By doing this, no conflicts/merge were asked for all its children. The 2 objects "Camp" were apparently not considered the same. No idea why, both are almost similar (very few changes here). And thus they are not merged together.