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

Implement numeric blender-style transforms. #58389

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

rcorre
Copy link
Contributor

@rcorre rcorre commented Feb 21, 2022

Implement numeric blender-style transforms.

This allows the user to input numbers during an "instant" (blender
style) transform operation to specify exactly how far to transform the
object. For example:

  • g2.5xx: Translate 2.5 units along the local x-axis
  • ry-45: Rotate -45 degrees around the y-axis
  • s.25Z: Scale by a factor of .25 on the xy plane

This removes any "{X,Y,Z}-Axis Transform" messages. These prevented the
"Transforming: (x,y,z)" messages from showing, and the latter are more
useful, as they tell you the actual units.

This also rearranges finish_transform to clear _edit before updating
the axis rendering, so an axis doesn't remain highlighted.

This is separated into two commits for ease of review, I'll squash before merging.

09bb986a38b2ffa3ace868f2cc4d2306229b47e2 refactors some common code for reuse.
017be8045469e4f1c68c1fe299caa4be1acbf10b implements numeric transforms leveraging that shared code.

EDIT: These have now been squashed.

@rcorre rcorre requested a review from a team as a code owner February 21, 2022 12:55
@@ -2057,6 +2051,7 @@ void Node3DEditorViewport::_sinput(const Ref<InputEvent> &p_event) {

if (k->get_keycode() == Key::SPACE) {
if (!k->is_pressed()) {
// TODO: Is this unreachable?
Copy link
Contributor Author

@rcorre rcorre Feb 21, 2022

Choose a reason for hiding this comment

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

I noticed this while trying to add a SPACE keybind. If !k->is_pressed(), we already return early above, so I don't think it is reachable, or know what it is supposed to do.

update_transform_numeric();
} else if (code == Key::PERIOD && _edit.numeric_next_decimal == 0) {
_edit.numeric_next_decimal = -1;
} else if (code == Key::SPACE || code == Key::ENTER) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better to use exact keycodes or the ui_accept and ui_select actions?

@Kemeros
Copy link

Kemeros commented Jul 26, 2022

@akien-mga @JFonS

Dunno if you've seen this one. Would be awesome to have this.

@YuriSizov YuriSizov modified the milestones: 4.0, 4.1 Feb 9, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally (without rebasing), it mostly works as expected. This is a feature I'm eager to see, great work so far 🙂

There are two significant issues I noticed on a Linux + AZERTY setup:

  • Numpad keys can't be used to enter numbers, regardless of Num Lock status. This also applies to the minus symbol to invert the operation (which works in Blender on my end).
  • Number row keys can be entered without pressing Shift (like on QWERTY), while AZERTY normally requires this. This is inconsistent with Blender's behavior on the same setup (tested on Blender 3.5).
    • Blender does not allow pressing 6 without Shift to invert the operation though (this maps to - on AZERTY). I think we should support that on AZERTY 🙂

For this PR to be most usable as a daily driver, I think both issues need to be fixed before it's merged.

@rcorre rcorre requested a review from a team as a code owner August 6, 2023 19:41
@rcorre
Copy link
Contributor Author

rcorre commented Aug 6, 2023

master had diverged quite far. I ended up re-implementing this rather than trying to make sense of the merge conflicts, so it might merit a re-review.

I switched to using keycode instead of physical_keycode, and played around a bit after running setxkbmap fr. I think it works properly, but I'm not too familiar with AZERTY. I had quite a fun time trying to find the keys to get it back to normal 😆. Numpad input should work now as well.

I noticed something odd about scaling. If you scale an object to half size (using either blender-style or normal gizmo operations), the Godot UI says you've scaled it by -0.5 (rather than 0.5, as I'd expect). I wasn't sure if that was intentional. I left the message as-is, but kept the input behavior the same as blender. That is, if you type s0.75, you'll scale the object to 0.75 of it's original scale, and the UI will say you've scaled it by -0.25.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works better now, but some keys still don't work correctly:

  • All numpad keys work, except Numpad Minus which doesn't add a - symbol before the transformation.
  • Only these top row keys work when holding Shift: 2, 6 (inverts transformation while it should insert 6 instead), 7, 9, 0. When you don't hold Shift, all these keys work too, which is not expected on AZERTY. The . symbol on the main keyboard (i.e. not on the numpad) doesn't work either to add a decimal.

@rcorre
Copy link
Contributor Author

rcorre commented Aug 7, 2023

Oops! I noticed some numbers not working but just figured my keyboard wasn't set up correctly.
I'm using unicode now, and as far as I can tell that works for all numbers on both us and fr layouts, and properly handles - and . both on and off the numpad.

I also noticed "-" wouldn't work if you pressed it before any numbers, so I fixed that.

@rcorre
Copy link
Contributor Author

rcorre commented Aug 7, 2023

Ah, conflicts again. I'll try to fix those tonight.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

AZERTY keys now work as expected 🙂

However, I noticed two other issues. I'm not sure if they were by today's changes, I may have started noticing them just now.

  • When you press - to reverse the direction, it applies to future transforms as well. This means that if you use GX-2 then use GY2, the node will be moved downwards by units because it's actually translating on the Y axis by -2, not 2. I'd expect it to only affect the current transform operation.
  • When you press - to reverse the direction, it doesn't display a minus symbol in the rotate text in the bottom-left corner. This does not occur with mouse-induced rotation, or with the translate/scale numeric transforms. This is purely a label issue – it doesn't affect how the rotate numeric transform actually behaves.

@rcorre
Copy link
Contributor Author

rcorre commented Aug 9, 2023

Thanks again! I've fixed the labeling and the lingering - state.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

It works great now! Thanks again for contributing this feature 🙂

@rcorre
Copy link
Contributor Author

rcorre commented Aug 10, 2023

Thanks for all the careful review! I've applied all the comment suggestions.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks good from a code style perspective (also checked overall of code complexity / architecture). Further feature/implementation assessment still welcome from the editor team.

@KoBeWi
Copy link
Member

KoBeWi commented Aug 10, 2023

When you type something and press Escape, the transform operation stops, but gizmos are no longer usable.

godot.windows.editor.dev.x86_64_D3niIExq7b.mp4

EDIT:
I'm not familiar with Blender, but shouldn't mouse motion be ignored after text input? Otherwise you can accidentally move mouse and there is no way to undo it.

This allows the user to input numbers during an "instant" (blender
style) transform operation to specify exactly how far to transform the
object. For example:

g2.5xx: Translate 2.5 units along the local x-axis
ry-45: Rotate -45 degrees around the y-axis
s.25Z: Scale by a factor of .25 on the xy plane

Some shared code between the traslate/rotate/scale branches of update_transform
was refactored into apply_transform so numeric transforms could reuse it.

This removes any "{X,Y,Z}-Axis Transform" messages. These prevented the
"Transforming: (x,y,z)" messages from showing, and the latter are more
useful, as they tell you the actual units.

This also rearranges finish_transform to clear _edit before updating
the axis rendering, so an axis doesn't remain highlighted.

Co-authored-by: Rémi Verschelde <[email protected]>
@rcorre
Copy link
Contributor Author

rcorre commented Aug 10, 2023

Thanks @KoBeWi, I've fixed both of the issues you mentioned.

@akien-mga akien-mga merged commit 48a447b into godotengine:master Aug 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@geekley
Copy link

geekley commented Dec 5, 2023

For anyone confused on how to test those, the shortcuts Begin * Transformation need to be assigned in the editor.


Some issues (as of v4.2) and suggestions for this feature:

  1. The Translating(...) message quickly disappears instead of staying the whole time the input is being entered. It's also not intuitive feedback like in Blender, where at least a | is shown where you're currently typing the number.
  2. Can't use backspace to delete the number being input, nor <- and -> arrows to navigate the input; like in Blender.
  3. Can't use tab (like in Blender) or , to input multiple axes (XYZ) in one operation.
  4. It would be really useful if math calculations were evaluated like in the inspector. E.g. r(,360/5) for Y-rotating 1/5 of a turn without having to think how much that is, etc.

IMO it would be much more intuitive if some actual text input field was shown during the operation. E.g.:
Pressing the key to begin a translation would show a text field pre-filled with t(|) allowing you to freely edit the text (here, | represents the cursor); so you could just type (e.g. t(2.5,-3) with z=0 implied). Then a simple parsing would apply the operation (possibly evaluating math expressions too). This would make this feature more powerful, a bit like CAD software (see AutoCAD Dynamic Input).

If feature 4 would cause incompatible behavior with current implementation (e.g. pressing - would just type - instead of negating the current value), then it could be either a separate feature, or separate mode setting for this feature. In any case, you need to have good feedback on what you're doing, otherwise it's not intuitive and not good usability. A text field is the best usability for inputting specific numeric values. It would be easier to understand too.

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.

7 participants