Skip to content
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

Sprite3D.isFlat() calculation does not take transformationMatrix3D adjustments into consideration #969

Open
Snky opened this issue Jun 4, 2017 · 7 comments
Labels

Comments

@Snky
Copy link

Snky commented Jun 4, 2017

..container = new Sprite3D();
_containerTransformationMatrix3DHelper = ..container.transformationMatrix3D.clone();
_containerTransformationMatrix3DHelper.(prepend/append)Rotation(rX, Vector3D.X_AXIS);
_containerTransformationMatrix3DHelper.(prepend/append)Rotation(rY, Vector3D.Y_AXIS);
_containerTransformationMatrix3DHelper.(prepend/append)Rotation(rZ, Vector3D.Z_AXIS);
..container.transformationMatrix3D.copyFrom(_containerTransformationMatrix3DHelper);

trace(..container.is3D, ..container.isFlat); //true, true

The isFlat function should compensate for these adjustments. Which would make the trace result: true, false

@b005t3r
Copy link

b005t3r commented Jun 4, 2017

How is it supposed to know that you changed the internals of the returned transformation matrix? If you used the rotationX/Y/Z properties, this would be reflected in isFlat returned valued.

@Snky Snky changed the title Sprite3D.isFlat() calculation does not take Matrix3D rotations into consideration Sprite3D.isFlat() calculation does not take transformationMatrix3D adjustments into consideration Jun 4, 2017
@Snky
Copy link
Author

Snky commented Jun 4, 2017

I understand that it wouldn't be aware of the translation/rotation/scale applied, but it could potentially be aware of the difference between the original transformationMatrix3D and the new one returned after the adjustments? Since the returned transformationMatrix3D is technically no longer 'flat'.

I have a given scenario where it is necessary to avoid making these adjustments directly on the container (via: x, y, z, rotationX/rotationY/rotationZ, scaleX/scaleY/scaleZ).

@PrimaryFeather
Copy link
Contributor

PrimaryFeather commented Jun 5, 2017

Hm ... I see what you mean. The problem is that there is no way for me to get notified that the transformation matrix was changed, so I can't trigger any logic that re-evaluates isFlat (which, furthermore, only looks at the Sprite3D properties, not the matrix).

One way to fix this would be an additional method (like setTransformationMatrix3DChanged) to be called manually after modifying that matrix. Or I could add an isFlat setter so you could manually indicate that it's non-flat, despite the values of the orientation properties.

Would something like that work for you, @Snky?
@b005t3r, any suggestions / preferences?

@b005t3r
Copy link

b005t3r commented Jun 5, 2017

But does it make sense to add anything like that? 3D matrix is a product of the 2D matrix and additional properties, modifying it like that is simply making a hack in my opinion. I think the simplest thing to do for Snky would be to override isFlat in a subclass and make it always return false.

The best way would be to create a setter for transformationMatrix3D and once it's called extract all rotations from it, BUT it's not always possible for 3D transformations (https://www.opengl.org/discussion_boards/showthread.php/159215-Is-it-possible-to-extract-rotation-translation-scale-given-a-matrix), so this is not an option either.

So, my vote goes to: create a subclass and override isFlat. Alternatively creating something like a setter or another property which would turn off isFlat's logic would work as well, but I think this would just create a backdoor for a more hacky approach to the framework, so I'd still rather go with the subclassing approach.

BTW, @Snky, if you don't want to use the build-in properties, maybe it's because of the same thing I discussed with Daniel a couple of days ago: #954 (read through the whole thing) and what you need is a new set of properties which change the Sprite's appearance. If so, what we agreed on in that issue's comments is exactly the thing you need (it'll require subclassing as well).

@Snky
Copy link
Author

Snky commented Jun 5, 2017

'isFlat' setter sounds like a non-abusive approach. 🍡😄

//Potential issue with a manual configuration of 'isFlat' (if a setter is available):

..container.isFlat = false;
trace(..container.is3D, ..container.isFlat); //true, false

..container.z = 10;
trace(..container.is3D, ..container.isFlat); //true, false

..container.z = 0;
trace(..container.is3D, ..container.isFlat); //true, (?) 

@b005t3r
Copy link

b005t3r commented Jun 5, 2017

Setter sounds like exposing internal logic as a part of public interface :) Something like useIsFlatLogic or overrideIsFlatValue would be better, but still far from perfect. isFlat as it is now is the best approach I can think of - it's validated when it needs to be validated by the internals (so you don't have to worry about switching its value in the right moment) and you can change the way it behaves a subclass if you wish.

@PrimaryFeather
Copy link
Contributor

Thanks for the feedback, guys!

I don't actually consider modifying the 3D transformation matrix a hack, or at least no more so than on the standard (2D) DisplayObject. However, different to 2D, it does have several side effects, one being that the isFlat optimization needs to be taken into account; another being that the 2D matrix must be the identity matrix as soon as the 3D matrix is non-flat.

Agreed, while an isFlat setter would do the trick, it is not very intuitive (as your sample shows, @Snky). As soon as its set to false, it would always return false; but if you set it to true, standard behavior would be restored, and the getter would return true or false depending on the other properties. Which is really awkward.

So it would need to be an additional property or method, in my opinion. But for now, @Snky, I'd say you try the subclassing approach first! Simple create a subclass of Sprite3D and override the isFlat getter to always return false. That should do the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants