-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Update GridMap to use Vector3i instead of three ints #39958
Conversation
@@ -524,8 +527,8 @@ void GridMapEditor::_set_clipboard_data() { | |||
|
|||
ClipboardItem item; | |||
item.cell_item = itm; | |||
item.grid_offset = Vector3(i, j, k) - selection.begin; | |||
item.orientation = node->get_cell_item_orientation(i, j, k); | |||
item.grid_offset = Vector3(ijk) - selection.begin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I thought that's some kind of C++ syntax which allows to group arguments. 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see how this can be confusing from the diff, but I think it's clear when reading the code as a whole.
Do you have an alternative name suggestion? I just called it "ijk" because it can easily be understood as "[the vector containing] i, j, and k".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of this, perhaps selected
or pos
would be better IMO, unless it shadows other variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I'd suggest selected
or pos
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, coming back to it I think selected
makes a lot more sense, since these are all in a selection.
We should avoid pos
since many instances of pos
were switched to position
in Godot 3.0, I think we should finish this work by converting the remaining instances of pos
to position
in Godot 4.0.
30f8fb1
to
8a333cd
Compare
Thanks! |
Seems to work well, but needs testing.