Fixed root-level game object transforms inside referenced collections when building from the editor #7504
Conversation
matgis
added
bug
editor/src/clj/editor/pose.clj
Outdated
| (defn- significant-translation? [value] | ||
| (if-some [[^double x ^double y ^double z] value] | ||
| (not (and (zero? x) | ||
| (zero? y) | ||
| (zero? z))) | ||
| false)) | ||
|
|
||
| (defn- significant-rotation? [value] | ||
| (if-some [[^double x ^double y ^double z ^double w] value] | ||
| (not (and (zero? x) | ||
| (zero? y) | ||
| (zero? z) | ||
| (== 1.0 w))) | ||
| false)) | ||
|
|
||
| (defn- significant-scale? [value] | ||
| (if-some [[^double x ^double y ^double z] value] | ||
| (not (and (== 1.0 x) | ||
| (== 1.0 y) | ||
| (== 1.0 z))) | ||
| false)) |
There was a problem hiding this comment.
I think the nil check semantically should belong higher up the stack, e.g. in a place where some pose attribute might be nil, we do the check before calling these predicates.
There was a problem hiding this comment.
Good point! I'll fix.
| (defn to-map [^Pose pose translation-key rotation-key scale-key] | ||
| {:pre [(pose? pose)]} | ||
| {translation-key (.-translation pose) | ||
| rotation-key (.-rotation pose) | ||
| scale-key (.-scale pose)}) |
There was a problem hiding this comment.
Maybe we should instead change the code that uses to-map to expect :translation/:rotation/:scale keys? I worry that this fn enables us to introduce more inconsistencies in pose-related code. The pose record is already a map...
Or maybe the pose should use names :position/:rotation/:scale3?
There was a problem hiding this comment.
These inconsistencies are unfortunately placed upon us by the requirements of the existing file formats. Scale is named :scale3 in collections because we used to have a uniform :scale that was a single float, but for game objects the non-uniform (i.e vector triplet) scale field is simply named :scale. Likewise, the translation field for both game objects and collections are named :position, even though it is in fact a translation along the (possibly rotated and scaled) axes of its parent. It is correctly (in my opinion) named :translation in the protobuf Transform message, which our Pose type use-case mirrors.
I would have preferred to call the Pose record Transform in the editor as well. Unfortunately the term transform is used to refer to Matrix4 values everywhere in the editor, and both the Spine and Rive extensions already rely on the transform output of the base SceneNode they subclass to produce a Matrix4. Maybe we can clean this up in the future, but we'd have to do it onto the dev since it is a breaking change requiring projects to update their library dependencies.
For now, I prefer to do the map conversion like this. In fact nothing outside of the unit tests make assumptions about what the record fields are called, or even that the Pose is represented as a record. If we ever get around to migrating away from javax.vecmath, we might want to represent it differently.
| {:pre [(or (nil? translation) (s/valid? ::translation translation)) | ||
| (or (nil? rotation) (s/valid? ::rotation rotation)) | ||
| (or (nil? scale) (s/valid? ::scale scale))]} |
There was a problem hiding this comment.
I think we should create and prioritize an issue about disabling asserts in production, and going through our asserts to separate dev-time contracts like these from user input validation. The spec library is somewhat slow...
There was a problem hiding this comment.
Yeah, we probably should. Disabling asserts did not seem to make a lot of difference at the time I profiled it, but I've started to add more checks since then.
But I'm not really worried about this particular case, since we only create Pose instances when producing build targets.
| (defn- rotate-vector [[^double vx ^double vy ^double vz] [^double qx ^double qy ^double qz ^double qw]] | ||
| (vector-of | ||
| :double | ||
| (- (+ (* vx qw qw) | ||
| (* vy qz qw -2.0) | ||
| (* vz qy qw 2.0) | ||
| (* vx qx qx) | ||
| (* vy qx qy 2.0) | ||
| (* vz qx qz 2.0)) | ||
| (* vx qy qy) | ||
| (* vx qz qz)) | ||
| (- (+ (* vx qx qy 2.0) | ||
| (* vy qy qy) | ||
| (* vz qy qz 2.0) | ||
| (* vx qz qw 2.0) | ||
| (* vy qw qw) | ||
| (* vz qx qw -2.0)) | ||
| (* vy qx qx) | ||
| (* vy qz qz)) | ||
| (- (+ (* vx qx qz 2.0) | ||
| (* vy qy qz 2.0) | ||
| (* vz qz qz) | ||
| (* vx qy qw -2.0) | ||
| (* vy qx qw 2.0) | ||
| (* vz qw qw)) | ||
| (* vz qx qx) | ||
| (* vz qy qy)))) |
There was a problem hiding this comment.
Did you write it yourself? I don't know how to verify this code...
There was a problem hiding this comment.
I looked at a couple of implementations of the formula, inlined a bunch of stuff, and reordered the coefficients in a way that made sense to me in prefix notation. I double-checked the math against both implementations I looked at to make sure I didn't mess it up, so I'm pretty confident it should be correct, but I guess time will tell! 😅
Fixes #7126.
User-facing changes
Technical changes
Posetype in places where it is important to retainposition,rotation, andscaleproperties.position,rotation, andscaleinto aMatrix4which we then decompose intoposition,rotation, andscalelater in the build process. Doing so lost information and resulted in transforms that differ from how Bob was doing it, which was applying parent transforms toposition,rotation, andscalein isolation.