Major refactoring of the Kubric internals #54
Conversation
Kubric.core now contains objects with traits (using the traitlets package) that are observed by the simulator and the renderer.
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
===========================================
+ Coverage 1.86% 15.26% +13.39%
===========================================
Files 13 19 +6
Lines 1070 1363 +293
===========================================
+ Hits 20 208 +188
- Misses 1050 1155 +105
Continue to review full report at Codecov.
|
|
@taiya Did you have a chance to look at this already? I would like to merge this PR soon, since it touches so much of the codebase and thus impacts all further development. |
|
Sorry, perf+promo delayed me quite a lot. |
1 similar comment
|
Sorry, perf+promo delayed me quite a lot. |
There was a problem hiding this comment.
Massive PR, so it was hard to cover everything; overall:
- use of traitlets → 👍
- removing double inheritance → 👍
- AttributeSetter → 👎 (not sure I enjoy the added logic)
For the latter (update of extra properties), I just wonder whether a simpler solution exists.
Further, having to do Blender(scene) (as opposed to inheritance) don't you immediately lose the "immediate render" mode? (more precisely, for blender it is immediate write to file, for THREEJS is actual immediate render)
Anyhow, as this is blocking you, and the internals are something we can improve over time, I'll approve.
Please see comments and auto-resolve/merge
| def __init__(self, target_obj, target_name): | ||
| self.target_name = target_name | ||
| self.target_obj = target_obj | ||
| self.mapping = None # has to be set by the add() function |
There was a problem hiding this comment.
add? not a member of this class? → confusde
There was a problem hiding this comment.
Yes, this is a bit ugly. It refers to the add() functions of the simulator / renderer.
There was a problem hiding this comment.
k, then it needs to be documented properly
| gravity = kt.Vector3D(default_value=(0, 0, -10.)) | ||
|
|
||
|
|
||
| class AttributeSetter: |
There was a problem hiding this comment.
Should this be here or in blender.py until it's used by multiple backends?
There was a problem hiding this comment.
We could move it there, though I am not sure in what way that would be better.
I intended to also use it for the simulator, but I ended up having to write specialised setters because pybullet.
There was a problem hiding this comment.
let's revisit this once we discuss the setters in detail
|
Great, thanks for the feedback @taiya! |
The main interface of Kubric now builds on the
traitletspackage.The idea is that for things like 3D objects or lights, there are corresponding classes in
core.py.They define traits for controllable parameters such as position or color.
Both PyBullet and Blender then register observers for those traits such that every change to that object is mirrored in simulation and rendering.