-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[geometry-1] Improve algorithm steps for identity transforms #579
Comments
The spec (and behavior in all browsers but Firefox) is also at odds with
Note that, if the angle of rotation around X or Y are zero, then It would be good for consistency if Firefox's behavior was adopted into the spec for With this inconsistency fixed in the spec, then the implementation of rotateAxisAngleSelf(x = 0, y = 0, z = 0, angle = 0) {
if (angle === 0) return this // <--------------- Firefox behavior.
toAxisRotation(axisRotationMatrix, x, y, z, angle)
this.multiplySelf(axisRotationMatrix) // "post multiply"
if (x !== 0 || y !== 0) this[is2D] = false // <---- without early return, all other browsers do this even if "nothing happened"
return this
}
rotateSelf(rotX = 0, rotY?: number, rotZ?: number) {
if (rotY == null && rotZ == null) {
rotZ = rotX
rotX = 0
rotY = 0
}
rotY ??= 0
rotZ ??= 0
this.rotateAxisAngleSelf(0, 0, 1, rotZ)
this.rotateAxisAngleSelf(0, 1, 0, rotY)
this.rotateAxisAngleSelf(1, 0, 0, rotX)
return this
} If we remove the early return from rotateAxisAngleSelf(x = 0, y = 0, z = 0, angle = 0) {
// if (angle === 0) return this // <--------------- Firefox behavior (commented out)
toAxisRotation(axisRotationMatrix, x, y, z, angle)
this.multiplySelf(axisRotationMatrix) // "post multiply"
if (x !== 0 || y !== 0) this[is2D] = false // <---- without early return, all other browsers do this even if "nothing happened"
return this
}
rotateSelf(rotX = 0, rotY?: number, rotZ?: number) {
if (rotY == null && rotZ == null) {
rotZ = rotX
rotX = 0
rotY = 0
}
rotY ??= 0
rotZ ??= 0
// the conditional checks prevent is2D from unnecessarily becoming false.
if (rotZ) this.rotateAxisAngleSelf(0, 0, 1, rotZ)
if (rotY) this.rotateAxisAngleSelf(0, 1, 0, rotY)
if (rotX) this.rotateAxisAngleSelf(1, 0, 0, rotX)
return this
} As you can see, to achieve the desired behavior of the spec'd |
I'm not sure how labeling works, but I labeled this as "geometry-1" because it is referring to that spec (and then if it gets modified, it would be "geometry-2" the version that has a change). Should this instead be labeled "geometry-2" because such a change would be for the next version? |
There's a discrepancy between browsers. This code:
Outputs different values across browsers:
matrix(1, 0, 0, 1, 0, 0)
matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1)
Technically all browsers except Firefox are "correct" because they do exactly what the spec says:
false
.However, Firefox's behavior is better: if the rotation is zero, then nothing needs to be done, the
rotateAxisAngleSelf
method can return early to avoid unnecessary work, and in this caseis2D
should remaintrue
because basically "nothing happened" due to the rotation being zero.I believe it would be best if the spec did the following, and maybe we can change the spec because browsers are not currently aligned, however it might be easier for a single browser (Firefox) to adopt the less desirable current spec behavior.
Here's what I would change the algorithm steps to:
false
.If "nothing happened", then there should be no effects, arbitrarily switching from 2D to 3D due to a zero-angle rotation is unnecessary.
We can also improve other algorithm steps, for example
translateSelf
can return early if x, y, and z translation values are all zero.This,
returns
matrix(1, 0, 0, 1, 0, 0)
in all browsers, which makes sense because "nothing happened" (and early returns would prevent unecessary work). Similar can be done withrotateSelf
,scaleSelf
, etc.The text was updated successfully, but these errors were encountered: