-
-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Improve editing of box collision shapes #71092
Conversation
ba81f45
to
e53f4cd
Compare
vvonderful |
b539cdc
to
03245dd
Compare
Jitter is fixed 🥳 godot.windows.editor.dev.x86_64_GRxjqDpOxY.mp4There's a small issue where the box can't be shrunk past its center position: godot.windows.editor.dev.x86_64_l0k4ybVF3C.mp4But this is caused by the fact that the result of Another thing is that there are at least 4 different gizmos for box editing and this PR handles only one. I plan to make a follow-up PR and unify this. And maybe do the same treatment for capsule and cyllinder shapes, because they have the same 2-side problem. |
When the shape resource is shared this will have the same issue as #73186, since the position is not part of the resource. I wonder how we should handle this, since I do like the otherwise improved editor experience. |
See my comment in that issue. |
Tested locally, it mostly works correctly. There are 2 issues I noticed:
Also, once the logic behind this is stabilized, this should probably be moved to reusable functions so that it can be implemented in similar gizmos like CSGBox3D, VoxelGI, ReflectionProbe and GPUParticlesCollisionBox3D. I would suggest rebasing first still as the code structure in |
As I mentioned before, it's a result of how
That's what I plan to do, but I wanted this PR to be merged first. |
Unless shape resources are going to be duplicated on copy by default, I think the convenience of this PR is not worth the confusion that will be caused by applying this operation to a shared shape resource. With that big side effect I don't think we can reasonably make this behavior the default. If it is implemented at all it should somehow be made clear that it affects (in a weird way) other resources sharing the same shape. |
The problem with shared shapes is that they change size unexpectedly, not that they are displaced. This PR doesn't make it any worse. |
03245dd
to
7062e7b
Compare
Ok rebased. There is one thing that I think could be improved tho. Right now I'm setting EDIT: |
7062e7b
to
6d7a629
Compare
Works great now! The only issue I noticed is that the snap length is halved when not extending relative to the center by holding Alt. (For instance, snap length is 0.5 units when it should be 1 unit.) |
Unfortunately I don't know how to fix the half snapping. If I simply multiply by 2, I end up with some weird offset. I tried different combinations, but none made it work correctly :/ |
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.
Tested locally (rebased on top of on top of master
dfebfd1), it works as expected.
Code looks good to me at a glance. Given how much this improves usability, I think this can be merged now and @KoBeWi can take a look at making this code more generic to work with similar 3D gizmos.
simplescreenrecorder-2023-08-01_17.54.51.mp4
Probably worth a rebase before merging as this was last updated in April. |
6d7a629
to
3eda886
Compare
3eda886
to
0a9a8c7
Compare
Thanks! |
Thank you!! This is super helpful |
Will this apply to CSG as well? If not that would be exceptionally helpful during the prototyping stage. |
There was a follow-up that applied it to every existing box gizmo: #80278 |
Awesome, thank you. This is a well appreciated addition. |
this is awesome!!!! this is something ive wanted for so long |
Closes godotengine/godot-proposals#452
There is still some weird jitter issue I need to fix.
EDIT:
Closes godotengine/godot-proposals#5669