Fix scaled parent relation in T46527.
Details
Diff Detail
- Repository
- rB Blender
- Branch
- soc-2014-bge-scenegraph_cleanup
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 I think it is clear for the true to be passed explicitly in this case. The default is false. | |
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. | |
source/gameengine/SceneGraph/SG_Spatial.h | ||
116 | yes, parent was removed from the function signature. |
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. |
source/gameengine/Ketsji/KX_SG_NodeRelationships.cpp | ||
129 | Not reported ones. | |
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. | |
source/gameengine/SceneGraph/SG_Spatial.cpp | ||
157 | here it is: http://lists.blender.org/pipermail/bf-committers/2014-July/043923.html |
source/gameengine/Converter/BL_BlenderDataConversion.cpp | ||
---|---|---|
2177 | Checking this again: | |
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. |
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? |
source/gameengine/Converter/BL_BlenderDataConversion.cpp | ||
---|---|---|
1921 | The world coordinates are initialized for all the scenegraph in So, there is no need to do it here.
|
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.
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 ? |
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.