scenegraph cleanup
AcceptedPublic

Authored by Inês Almeida (brita_) on Jul 25 2014, 8:39 PM.

Diff Detail

Repository
rB Blender
Branch
soc-2014-bge-scenegraph_cleanup
Inês Almeida (brita_) retitled this revision from to scenegraph cleanup.Jul 25 2014, 8:39 PM
Inês Almeida (brita_) updated this object.
Inês Almeida (brita_) set the repository for this revision to rB Blender.

Did an initial code review pass. There are a few methods I haven't looked through yet, including: bl_ConvertBlenderObject_Single, KX_BoneParentRelation:UpdateChildCoordinates, KX_NormalParentRelation:UpdateChildCoordinates, and KX_SlowParentRelation:UpdateChildCoordinates. Most of the in line comments are probably answered by commit messages, but it wouldn't hurt to answer them here as well.

source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp
468 ↗(On Diff #2220)

Is this initialized somewhere else (such as by a calloc)?

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2177

Probably better to stick with NodeUpdateGS to follow the Law of Demeter: http://en.wikipedia.org/wiki/Law_of_Demeter

source/gameengine/Converter/KX_BlenderSceneConverter.cpp
670 ↗(On Diff #2220)

Perhaps we should leave the old code as a guide? I suppose version control can allow someone to dig up the old code.

source/gameengine/Ketsji/KX_GameObject.cpp
128

Why get rid of the comment?

source/gameengine/Ketsji/KX_PhysicsEngineEnums.h
39

Can we just set Bullet to be 1, and not bother with this comment?

source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp
121

Why change this logic?

129

Was not setting parentUpdated and the worldScale causing any problems or bugs?

source/gameengine/Ketsji/KX_SG_NodeRelationships.h
153

I don't like seeing behavior defined in a header file. I know it can be tedious to define the behavior of a one line method in the cpp file, but it keeps things cleaner. I don't know if the Blender style guide has anything to say about this.

source/gameengine/SceneGraph/SG_Spatial.cpp
157

Why was the case with a parent removed?

source/gameengine/SceneGraph/SG_Spatial.h
116

Looks like part of this comment is now missing. Was that intentional?

source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp
468 ↗(On Diff #2220)

addInitFromFrame was removed

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2177

UpdateWorldData needs time elapsed=0, and parentUpdated=true (second parameter) that will go all the way to
https://developer.blender.org/diffusion/B/browse/soc-2014-bge/source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp$50
as true, meaning that 'the parent of the root node is updated' -> meaning a force calculate world coordinates / the world has changed. Makes sense for the initial computation.

I think it is clear for the true to be passed explicitly in this case. The default is false.
Maybe it shouldn't be?

source/gameengine/Converter/KX_BlenderSceneConverter.cpp
670 ↗(On Diff #2220)

I can put the old cold back in with #if 0

source/gameengine/Ketsji/KX_GameObject.cpp
128

I didn't think it was adding anything

source/gameengine/Ketsji/KX_PhysicsEngineEnums.h
39

I think it must be 5 for backwards compatibility

source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp
121

I just reordered it in a clear way, consistent for all updateCoordinates implementations

129

I don't know what you are talking about here

source/gameengine/Ketsji/KX_SG_NodeRelationships.h
153

In this case, it is just getter/setters

source/gameengine/SceneGraph/SG_Spatial.cpp
157

RelativeTranslate and Rotate, according to the documentation, should transform either in local coordinates, or in world coordinates. Never the parent's.
I emailed about this to bf-committers

source/gameengine/SceneGraph/SG_Spatial.h
116

yes, parent was removed from the function signature.
That part of the comment is relative to the parent

source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp
468 ↗(On Diff #2220)

Ok.

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2177

I wonder then if NodeUpdateGS should take an optional parameter for parentUpdated that defaults to false. That should avoid breaking anything, and allow you to still have it set to true for this case.

source/gameengine/Converter/KX_BlenderSceneConverter.cpp
670 ↗(On Diff #2220)

Is this a function that is even used anymore? If we can safely remove the contents of the function, can we not just remove the function entirely? If the function is still useful, then perhaps you should rewrite the functionality before merging to master.

Whatever we decide to do with this function, we should be consistent with KX_BlenderSceneConverter::ResetPhysicsObjectsAnimationIpo, as that function appears to suffer from the same problem.

source/gameengine/Ketsji/KX_GameObject.cpp
128

Ok.

source/gameengine/Ketsji/KX_PhysicsEngineEnums.h
39

If something breaks by changing the numbers in an enum, we either need to fix that, or add a comment to warn others about changing the enum.

source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp
121

Ok.

129

It looks like the old code does not set worldScale or parentUpdated. Your new code does. Does this change any current behavior, such as fixing any bugs?

source/gameengine/Ketsji/KX_SG_NodeRelationships.h
153

I understand that they are just getters/setters, I still don't really like behavior being defined in the header file. I don't mind it staying if some others say it is ok, since I don't think this is really covered in the style guide.

source/gameengine/SceneGraph/SG_Spatial.cpp
157

Sorry, I must have missed that email. I'll go look it up and read through it.

source/gameengine/SceneGraph/SG_Spatial.h
116

Ok.

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2177

I can do that

source/gameengine/Converter/KX_BlenderSceneConverter.cpp
670 ↗(On Diff #2220)

Yes, both functions have the same problem, I talked about this with campbell at the time. The functions are supposed to be used, but both are doing nothing since a very long time.
He said that they need to be written again (not removed), and that the code was with #if 0 to serve as a basis to the next person, so I didn't touch it, but had already removed the contents of the second function on a previous commit.
I removed it when I removed addInitFromFrame, maybe it doesn't even serve as basis?
At the moment I don't know enough about IPO's to rewrite this, but I can take a look

source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp
129

Not reported ones.
They are correct changes, a vertex child inherits only the translation, but it still should apply it's local scale and orientation to itself. I didn't bother testing, but if the scale of a child was changed after the game conversion (that set's the world coordinates with normal parents before changing the type of parenting), the child would never change size.
And the children of a vertex child would likely not be updated in some cases (because updateParent wasn't being set to true, it is keeping the previous value), in some other cases the children would be recomputed with no need.

source/gameengine/Ketsji/KX_SG_NodeRelationships.h
153

I see this kind of thing everywhere, I didn't put it there, just reformatted. I can, off course, move it to the cpp.
Do you want to ask someone or I just move it?

source/gameengine/SceneGraph/SG_Spatial.cpp
157

here it is: http://lists.blender.org/pipermail/bf-committers/2014-July/043923.html
I have summed up the remaining questions in the scenegraph documentation page http://wiki.blender.org/index.php/Dev:Source/GameEngine/SceneGraph

source/gameengine/Converter/BL_BlenderDataConversion.cpp
2177

Checking this again:
The UpdateWorldData function is from SG_Node
https://developer.blender.org/diffusion/B/browse/soc-2014-bge/source/gameengine/SceneGraph/SG_Node.cpp;3f79ceb0eab3c9610e2fc688152291f0b443af5b$171
and used in several other places such as KX_Scene and the videotexture module.
The NodeUpdateGS is from KX_GameObject.
I don't think it hurts to call this function from the DataConversion as it knows about SG_Nodes.. it makes them.
However, this is a minor thing, I totally don't mind changing this to NodeUpdateGS and changing that to accept an optional parameter defaulting to false

source/gameengine/Converter/KX_BlenderSceneConverter.cpp
670 ↗(On Diff #2220)

I took another look at this, I still don't know enough to rewrite it, and there is lot of other similar functions.
I'm adding back what I removed as a comment for whomever does it

  • Corrections to the scenegraph cleanup according to review

Updating the scenegraph cleanup revision

I reviewed all the gsoc commits and reapplied everything branching from master.
There are some commits that were previously missing, and the addInitFromFrame removal commit is ommitted this time as it is not really related with scenegraph

  • Game Engine Scene Graph - whitespace cleanup
  • Game Engine Scene Graph - documentation cleanup
  • gameengine scenegraph: whitespace and comment cleanup
  • gameengine scenegraph: correction of relative postition computation, fixes T28908
  • gameengine scenegraph: whitespace and comment cleanup
  • gameengine scenegraph: whitespace and comment cleanup - KX_VertexParentRelation
  • gameengine scenegraph: small refactor to KX_NormalParentRelation , KX_VertexRelation was not marking changes for it's children
  • gameengine scenegraph: whitespace and comment cleanup - KX_SlowParentRelation
  • gameengine scenegraph: whitespace and comment cleanup - KX_BoneParentRelation
  • gameengine: scenegraph corrections
  • game engine: scenegraph cleanup
  • gameengine: more tweaks to the scenegraph
  • fixing slow parent motion in the game engine
  • slow parent in game engine: fix for negative values
  • attempting to fix slowparent rotation in the viewport

Looks good, I had one minor question that I inlined.

Be aware that changing so much code will likely lead to some bug reports. So be prepared to handle those.

source/gameengine/Converter/BL_BlenderDataConversion.cpp
1921

Why was this removed?

This revision is now accepted and ready to land.Jan 22 2015, 7:21 AM
source/gameengine/Converter/BL_BlenderDataConversion.cpp
1921

The world coordinates are initialized for all the scenegraph in
BL_ConvertBlenderObjects()
just bellow the comment that says:
/* Find all 'root' parents (objects that have no parents in SceneGraph) and init the world transforms */

So, there is no need to do it here.
I added a comment, 10 lines above here, saying:
/* Note: world coordinates are calculated for all nodes when the scene graph

  • is complete, after processing vec_parent_child */

Maybe, this clean-up can be committed just after 2.75 release (in a few weeks) to have all the cycle time to test it

I have detected at least 2 bugs already with this patch that I did not have time to fix.
I spent a day looking into it, but it needs more time, that is why it hasn't gone in yet.
The bug is visible with this file: F17105 after commit 4117efcde301 of this patch, which was originally a fix for that same file.
That commit does change the way a child's rotation is calculated, but I am not sure yet of what is wrong.

I haven't been having the time to look at this, and I don't think I will for the next 2 weeks either, but maybe after.

Porteries Tristan (panzergame) added inline comments.
source/gameengine/Converter/BL_BlenderDataConversion.cpp
2277

Why UpdateWorldData instead of NodeUpdateGS ?

source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp
168–227

@Inês Almeida (brita_) : Did you test this part, to avoid bugs ?

source/gameengine/Ketsji/KX_SG_NodeRelationships.h
216

Why remove this comment ?

Porteries Tristan (panzergame) retitled this revision from scenegraph cleanup to Fix T46527 : scenegraph cleanup.Oct 27 2015, 8:50 AM
Porteries Tristan (panzergame) updated this object.
Porteries Tristan (panzergame) retitled this revision from Fix T46527 : scenegraph cleanup to scenegraph cleanup.

If the code apply world scale to local position before rotating the issue seems fixed.

Using:

child->SetWorldPosition(p_world_pos + p_world_rotation * (p_world_scale * child->GetLocalPosition()));

instead of:

child->SetWorldPosition(p_world_pos + p_world_scale * (p_world_rotation * child->GetLocalPosition()));

I don't know if this fix is theorically correct…

@Inês Almeida (brita_) , @Jorge Bernal (lordloki): Could you try and confirm please ?

@Inês Almeida (brita_), @Jorge Bernal (lordloki), I noticed something about the world scaling of the objects. It is computed by only multiplying the local scale of parent and child objects without taking account of the rotation. So if we have an object without children, the world scale is the local scale even if the object is rotated which seems doubtful as world should be for world space…

In case of object with children objects, the world scale of the child is equivalent of the local scale of the child multiplied by the world scale of the parent object, so finally local scale of the parent multiplied with child local scale, this looks even more doubtful when parent and child use different orientation.

On my side i will try to make the world scale as the local scale of the object in world space.