Closed greyepoxy closed 4 years ago
I changed DrawDevice.ViewerAngle into DrawDevice.ViewerOrientation, should I leave ViewerAngle alone to make sure this is not a breaking change?
We can't really make breaking changes unless we're doing a major version step, so the best we could do is add a new property in addition to ViewerAngle
, like ViewerTilt
. It's still breaking to add new interface members, but it's probably safe to say that the Duality core itself is currently the only implementer of that interface, we should be good.
Setting the Camera.Tilt in the editor over or under 360 does not wrap in the same way that the Transform.Angle does. Seems like there is some special editor logic in https://github.com/AdamsLair/duality/blob/master/Source/Plugins/EditorBase/PropertyEditors/Components/TransformPropertyEditor.cs#L220 that does that. Do we want that for this Tilt value as well?
Since we're dealing with the special case of a tilt away from a nominal angle, I'd suggest using a range of [-90, 90] and clamping the UI value to that range using a EditorHintRange
attribute on the property. By limiting to 90° away, and not 180°, we're also confining the camera to one side of the focus plane, avoiding situations where it would look at the back sides of everything.
Currently storing the three camera tilt/orientation values as Euler angles and then applying them to the view matrix in a very specific order, rotation around Z axis first, then around Y, then X. Probably would be better to store the rotation as quaternions as that should reduce the cost of producing the rotation matrix.
I think euler angles are completely fine - we're not doing that much heavy math here, performance impact should be negligible and eulers are easier to deal with here. Adding to this, we only need a Vector2
here, since the Z axis is already defined by this.Transform.Angle
.
Related to the item above (as in we will probably want to do that if we want to do this item), there is a bunch of helper methods in the Transform object for performing angle changes, do we want to offer some similar methods for Camera tilt? If Camera shake is something we want first class support for, then this is probably a good idea.
Which methods do you have in mind?
Okay great! Thank you for your thoughts and feedback, I updated the change to keep tilt
separate from angle
in both the camera and drawDevice public interfaces. So only adding to the interface now.
I have mixed feelings on limiting the rotation to [-90, 90] while I believe you are completely correct that it would cover the scenarios I had in mind. I think there are a couple cases you would want to go outside of this. For example if you wanted to flip the world on its head, an easy way to do that would be to flip the camera 180 degrees along its viewing plane. If we limit to [-90, 90] then that would not be possible. Not sure if this is compelling enough to allow it though.
Here were the additional functions that I was considering for the Camera object,
This might look to much like 3d, so would be happy to start with something simpler if desired
Sorry for not responding earlier - for time reasons, I wasn't able to continue looking into this as needed.
@Barsonax @SirePi Can you guys pick up this PR? Feel free to ping me if needed down the line.
Just my personal preference maybe, but I think that instead of having 6 methods
- RotateRight(float angle)
- RotateLeft(float angle)
- RotateUp(float angle)
- RotateDown(float angle)
- RollLeft(float angle)
- RollRight(float angle)
with kind of "unreferenced" names like left/right/up/down, and with the potential headache of having to consider that RotateRight(-5) is equal to RotateLeft(5), maybe it would be better and easier (to understand and maintain) to have
RotateRight(float angle) + RotateLeft(float angle) => RotateY(float angle)
, limited to [-90, 90]RotateUp(float angle) + RotateDown(float angle) => RotateX(float angle)
, limited to [-90, 90]RollLeft(float angle) + RollRight(float angle) => RotateZ(float angle)
, full 360° ?@greyepoxy There have been quite a bit of changes in master recently. Can you rebase your branch on master so its up to date again? The changes are mostly in the csproj files so theres a good chance you won't have conflicts.
Hey all thanks for your replies, unfortunately due to some changes in my available time and priorities I won't be able to continue working on this. Feel free to build off of my work if you would like but I totally understand if its not a priority for Duality at the moment.
Again I really appreciate everyone's time and what you all are building here
Ok thanks for letting us know and thanks for the effort you put in.
I pulled your branch into this repo and we will handle things further from now once we find some time.
This is a work in progress
This is an initial PR for https://github.com/AdamsLair/duality/issues/747
The core of the functionality works for anyone that wants to play around with it. Add some things to the scene and change the camera transform angle and camera tilt values to see the world rotate around you.
TODOs:
GetVisibleTileRect
is probably broken with this change, need to investigate a fix.Things to discuss:
DrawDevice.ViewerAngle
intoDrawDevice.ViewerOrientation
, should I leaveViewerAngle
alone to make sure this is not a breaking change?