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

Fix some Node3DEditor snapping issues #84049

Merged
merged 1 commit into from
Oct 31, 2023

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Oct 27, 2023

Fixes #79862.

There was a discrepancy between what's saved in the plugin's state and where it gets loaded to:

d["translate_snap"] = get_translate_snap();
d["rotate_snap"] = get_rotate_snap();
d["scale_snap"] = get_scale_snap();

if (d.has("translate_snap")) {
snap_translate_value = d["translate_snap"];
}
if (d.has("rotate_snap")) {
snap_rotate_value = d["rotate_snap"];
}
if (d.has("scale_snap")) {
snap_scale_value = d["scale_snap"];
}

All get_translate_snap/get_rotate_snap/get_scale_snap were reading the values from the snap dialog's LineEdits instead of using plugin's snap_translate_value/snap_rotate_value/snap_scale_value (which are being updated according to these LineEdits on pressing OK / confirming in the snap dialog). Also what's worse (given these were the values being saved), these methods return smaller snap values when Shift is being pressed.

double Node3DEditor::get_translate_snap() const {
double snap_value;
if (Input::get_singleton()->is_key_pressed(Key::SHIFT)) {
snap_value = snap_translate->get_text().to_float() / 10.0;
} else {
snap_value = snap_translate->get_text().to_float();
}
return snap_value;
}
double Node3DEditor::get_rotate_snap() const {
double snap_value;
if (Input::get_singleton()->is_key_pressed(Key::SHIFT)) {
snap_value = snap_rotate->get_text().to_float() / 3.0;
} else {
snap_value = snap_rotate->get_text().to_float();
}
return snap_value;
}
double Node3DEditor::get_scale_snap() const {
double snap_value;
if (Input::get_singleton()->is_key_pressed(Key::SHIFT)) {
snap_value = snap_scale->get_text().to_float() / 2.0;
} else {
snap_value = snap_scale->get_text().to_float();
}
return snap_value;
}

Meaning if Shift was pressed when plugin's state was being saved then values smaller than the actual snap values were being saved into a file. Then when loading they'd be loaded as the actual snap values. Everytime this happened the snap values were becoming smaller and smaller.

For reproducing #79862 it was enough for me to create two 3D scenes and keep switching between them (by pressing their tabs) while keeping Shift pressed.

This PR makes get_translate_snap/get_rotate_snap/get_scale_snap properly use snap_translate_value/snap_rotate_value/snap_scale_value, and makes snap_translate_value/snap_rotate_value/snap_scale_value be what's saved in the plugin's state.

Since get_translate_snap/get_rotate_snap/get_scale_snap no longer read values from LineEdits they were changed to return real_t instead of double, as that's the type of snap_translate_value/snap_rotate_value/snap_scale_value. See #49783 (comment) for why they were returning doubles before.
If we'd want snapping to always operate on doubles then what would need to be changed are snap_translate_value/snap_rotate_value/snap_scale_value. I could make such changes if that's desired.


Fixes #39507.

angle = Math::rad_to_deg(angle) + snap * 0.5; //else it won't reach +180
angle -= Math::fmod(angle, snap);

Math::fmod would return NaN for snap == 0.0. Besides, this calculation doesn't correctly snap for negative angles anyway.
Fixed it to use Math::snapped which handles these cases properly.

@kleonc kleonc added this to the 4.2 milestone Oct 27, 2023
@kleonc kleonc requested a review from aaronfranke October 27, 2023 11:12
@kleonc kleonc requested a review from a team as a code owner October 27, 2023 11:12
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tested this but it looks good.

@akien-mga akien-mga merged commit f1f92bd into godotengine:master Oct 31, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@rcorre
Copy link
Contributor

rcorre commented Oct 31, 2023

Wow, great find! This was driving me crazy!

@kleonc kleonc deleted the node3d-editor-plugin-snapping branch October 31, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor 3d transform snap parameters randomly become very small Rotation with snap sets values to NaN
4 participants