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

Rotation by input & snap angle input #978

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

HellAholic
Copy link
Contributor

@HellAholic HellAholic commented Nov 25, 2024

Add rotation by input fields

  • Fields only active when snap rotation is disabled
  • Fields take user input, rotate the model and return to value 0
  • Getting model rotation or storing the rotation values lead to unexpected behavior (read leave Quaternions alone)
  • Includes the snap rotation PR (Add rotation snap amount to UI #979)
  • Adds setter and getter for RX,RY,RZ (rotations in each axis)
  • Applies the rotations for single or multiple model(s)

Rotate by input
rotation_by_input1

Snap angle update
Based on the idea of GregValiant
snap_angle

barely functional, requires:
- A way to store and retrieve rotation angle for x, y, z
- Update the getters to pass the rotation x, y, z back to the qml
- Tidy up and remove duplicates -> make code generic
Retrieve the node quaternion rotation, try to get the rotation on axis from the data
getter will return the value in degrees to be consumed in the qml
setter will store value in radians to remain consistent if the value is in use elsewhere.
@GregValiant
Copy link
Contributor

@nallath Any thoughts on this one? It sort of goes hand-in-hand with the "Y" axis Translate tool.

Saving sanity mostly
Y and Z are flipped between the model data and what is displayed for user.
Make comments more relevant to changes
- Life is short & figuring out Quaternions is not, instead of trying to get the value from object, get the value from user rotations and store the last rotation per axis
- Works for single and multiple objects being rotated at once
- Reset values with rotation reset
@HellAholic
Copy link
Contributor Author

Rotation_by_input

@GregValiant ☝️
after staring into the abyss (read giving up on Quaternions), I just made a simple workaround.

Final bit that needs to be added is resetting the value when another model is selected (switching between multiple models).
But works for almost every case (single model / multiple models)
Let me know if anything seems off/missing.

I was never here
@GregValiant
Copy link
Contributor

I thought you were on holiday. Couldn't stand it heh?

I was using the previous iteration and didn't have any issues. I've just brought in the latest and greatest (but not the Quaternion thing) and I'll let you know. At first glance it's fine.

Jaime had merged the PR for the translate tool "Y". I was looking into something else and saw a conflict with "one at a time". I have a comment in that PR #980 and I was wondering if you needed a break (tee hee hee) if you would look at that. He hasn't responded and I don't know if he's busy or he left on his holiday but I think it needs to be considered as right now it looks like a show-stopper to me.

@HellAholic
Copy link
Contributor Author

Yeah, something like that xD
Did the holiday bit and came back, had a "stop making life difficult" moment last night in the train and updated things in an hour or two xD
I'll take a look at the Y thing.

Input is empty, the user types in the value, rotation is applied and the field clears
Copy link

github-actions bot commented Dec 7, 2024

Test Results

2 407 tests   2 392 ✅  23s ⏱️
    1 suites     15 💤
    1 files        0 ❌

Results for commit ad5366c.

♻️ This comment has been updated with latest results.

Removing elements leads to unstable behavior, so rather have a solid base that just resets to 0 than a flaky front
@HellAholic HellAholic marked this pull request as ready for review December 7, 2024 19:35
@HellAholic HellAholic added the PR: Community Contribution 👑 Community Contribution PR's label Dec 7, 2024
@HellAholic HellAholic changed the title Rotation by input Rotation by input & snap angle input Dec 7, 2024
@HellAholic
Copy link
Contributor Author

RotateTool.zip
@GregValiant give this one a try. Has both rotation by input and the snap angle change.

@GregValiant
Copy link
Contributor

It looks really good. It will rotate to any custom angle to two decimal places.

Here I've adjusted the angle to 11.25 so it will hit 22.5 right on the money.
image

@rburema
Copy link
Member

rburema commented Dec 11, 2024

I know we had something like this previously, but purposefully took them out because they'd mess up scaling, mirroring, etc.

For example:

rotation_and_scaling.mp4

@HellAholic
Copy link
Contributor Author

Thanks for the check and feedback. Did not check that part. I'll try to see if I can figure out what goes wrong where and either:

  • Take out rotation by input and just leave the snap rotation in (if that doesn't cause the same issue)
  • Fix the scaling??? (wishful thinking but a magical time is on approach)

@GregValiant
Copy link
Contributor

That looks like how it always acted because the bounding box doesn't rotate but rather stretches to fit the model. Any rotation must be the last transformation.
If you rotate the model first and then stretch it, it gets skewed because that's how it is in the bounding box.
This is in 4.13.1
https://github.com/user-attachments/assets/71112f15-5efb-4b56-9fb1-60ac625b05d7

@GregValiant
Copy link
Contributor

I think the only way around the weirdness that occurs when scaling a rotated body is to rotate the bounding box and have the part coordinate system tied to it. When you rotate the model, the local coordinate system rotates with it. I don't see that happening.
I have Cura 2.3 loaded on a desktop and the behavior is the same. Rotation must always be last.

@rburema
Copy link
Member

rburema commented Dec 12, 2024

You're right, it does already happen -- woke up way to early with the thought to check this, but you'd beaten me to it. -- That'll teach me to review stuff when I'm tired.

Anyway, I think this works because the rotation fields get set back to 0 here, we previously didn't have that (also tried to keep the rotation OK), and I think that may have caused the actual issue.

So the concept looks good (though I haven't looked at the code properly yet) -- I've only taken off a few days this year, so now I'm really going into my month-long holiday == see you on the other side! 👋🏻

@GregValiant
Copy link
Contributor

I'm sure this will still be sitting here when you get back.

I'd been using my bare bones version of the Rotation change (along with the Translation Tool "Y" flip) for a while, and I haven't seen any adverse behavior. I have the latest versions of both installed and it's all good.

Those two changes did require some head scratching (does Hellaholic have hair?) BUT altering "Quaternions" has been avoided. That's gotta be worth something right there.

The CAD packages I'm familiar with (Inventor and SolidWorks) do not store "current rotation angles" anywhere. The only way to get the current angle is to adjust the coordinate system the way you need it, and then to add a dimension to the workspace. The angle is always subjective to the view rather than being absolute.
The reason for that is (drum roll please) ... the Bounding Box doesn't rotate . It simply stretches square to the World Coordinate System to contain the extents of the model whatever they happen to be.

@HellAholic
Copy link
Contributor Author

HellAholic commented Dec 12, 2024

I'll add a ticket for the rotation & scale mix. Just to be safe 📦
rotation_scale_no_mix

@GregValiant
Copy link
Contributor

You need a model that is more fun.

GV_SupportShape.-.UltiMaker.Cura.2024-12-12.16-42-06.mp4

I think it just needs a tooltip change.
"Any model rotation must be the last transformation or the model can be subject to random wonkiness."

@HellAholic
Copy link
Contributor Author

HellAholic commented Dec 14, 2024

Had an idea to get around the issue, but not sure how practical it would be. It involves Quaternions. So the outline is:

  1. Get current model's Quaternion and store it (temp)
  2. Invert the rotation (reset rotation)
  3. Apply scale
  4. Re-apply rotation through the stored Quaternion or matrix

First need to get things working with Conan 2 xD

@GregValiant
Copy link
Contributor

"Re-apply rotation through the stored Quaternion or matrix"
What if the model is at a 2 or 3 angle compound rotation?

I'm catching up on my reviews. It's been about 3 weeks now and I haven't had any problems with this at all. The changes to the Translate Tool have likewise been fine.

@HellAholic
Copy link
Contributor Author

I'm trying to understand the scale behavior, but it doesn't seem to be as "Simple" as the rotation. The compound rotation is not the issue, at any given time, there is only one Quaternion that describes the model. It will get messy with the operations a bit since it adds a step but that wasn't the issue.
There are a lot of rounding and loss-prone calculations that are done during the scale, as far as I could figure out, so that makes it the same level of tricky as getting the value of rotation back (which I gave up on since it wasn't possible - infinite solutions) the scale goes the other way and stores the scale factor in x, y, z, but when you rotate the model, those values are no longer valid.

So a very long story short, it's a big project to refactor the scale, and cannot be a simple adjustment by me (QA) :P
But since it's separate from this PR, I'll leave it be for now, if I have a spark of inspiration or suddenly develop an urge to code, I'll pick it up again.

@GregValiant
Copy link
Contributor

GregValiant commented Dec 30, 2024

It would have to be a heck of an inspiration.

For now I'll stick with the thought that the bounding box has to rotate, and the Model Coordinate System must rotate with it. That would keep the Model Coordinate System always at right angles to the faces of the bounding box.

Everything else leads to "Quaternion Madness". which is inCurable.

(Yes, it's a horrible pun. No, I will not apologize.)

@GregValiant
Copy link
Contributor

GregValiant commented Dec 31, 2024

Because QT doesn't have both an "enabled" state and a "visible" state for controls, this is pretty clumsy.

I have noticed that the odd scaling behavior of a rotated model only occurs when "NonUniformScale" is True.

  • I rotate a model
  • That sets a new variable "HasBeenRotated" to True.
    • When "HasBeenRotated" is emitted - NonUniformScale is set to False and both the uniformScalingCheckbox and it's label are disabled.
    • That enables another checkbox and label which are dummies and are in the exact same location as the real ones. The checkbox is locked on True and the label explains that due to the rotation, scaling can only be "Uniform".
  • When I select the "Reset Rotation" button:
    • "HasBeenRotated" becomes False
    • The dummy checkbox and label disappear and the real ones show up again allowing the user to de-select Uniform Scaling.

If it was possible to "lock" the uniformScalingCheckbox and change the label text - the dummies wouldn't be needed.

This sort of thing is the reason I call myself the King of Workarounds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants