Skip to content

Gizmo Arrows #10550

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Nov 15, 2023
Merged

Gizmo Arrows #10550

merged 17 commits into from
Nov 15, 2023

Conversation

SIGSTACKFAULT
Copy link
Contributor

@SIGSTACKFAULT SIGSTACKFAULT commented Nov 14, 2023 •
edited
Loading

Objective

Solution

(excuse my Protomen music)

2023-11-13.22-30-00.mp4

Wasn't horribly hard when i remembered i can change coordinate systems whenever I want. Gave them four tips (as suggested by @alice-i-cecile in discord) instead of trying to decide what direction the tips should point.

Made the tip length default to 1/10 of the arrow's length, which looked good enough to me. Hard-coded the angle from the body to the tips to 45 degrees.

Still TODO

  • actual doc comments
  • doctests
  • ArrowBuilder.with_tip_length()

Changelog

  • Added gizmos.arrow() and gizmos.arrow_2d()
  • Added arrows to 2d_gizmos and 3d_gizmos examples

Migration Guide

N/A

@alice-i-cecile alice-i-cecile added A-Gizmos Visual editor and debug gizmos C-Feature A new feature, making something new possible labels Nov 14, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Nov 14, 2023
@alice-i-cecile alice-i-cecile requested review from nicopap and IceSentry and removed request for nicopap November 14, 2023 03:26
@alice-i-cecile
Copy link
Member

Basically looks good once the TODOs are addressed. Can you grab a screenshot or video for reviewers and the curious passer-by to check out and add it to your PR description?

@SIGSTACKFAULT
Copy link
Contributor Author

Can you grab a screenshot or video for reviewers and the curious passer-by to check out and add it to your PR description?

was already doing it <3

@SIGSTACKFAULT
Copy link
Contributor Author

Basically looks good once the TODOs are addressed.

ready!

@IceSentry
Copy link
Contributor

The size of the arrow head should probably be clamped. 1/10 does make sense, but if someone makes a really long arrow it will definitely end up looking weird. It's a bit hard to tell what should be the limit though.

Would be interesting to see how it looks if you also connect the base of the arrow head. To me it looks like there's something missing without a base.

@nicopap nicopap mentioned this pull request Nov 14, 2023
Open
57 tasks
@nicopap
Copy link
Contributor

nicopap commented Nov 14, 2023

I think it makes sense to keep the head proportional to the arrow length. In 3D, consider an arrow that is very far away, if you clamp the head size, it will look tinny. There is no good general algorithm to figure out a "good" clamped arrow head size. In any case, this PR allow controlling the "tip length".

nicopap
nicopap suggested changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@nicopap nicopap 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, just a minor grip.

SIGSTACKFAULT and others added 9 commits November 14, 2023 08:40
@SIGSTACKFAULT @nicopap
add arrow_head_length alias to with_tip_length
75e7752 
suggested by @nicopap

Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT @nicopap
make the comment on ArrowBuilder.drop a bit more professional
a93dc95 
Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT @nicopap
capitalize "Example" in three places
dba55c8 
Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT @nicopap
Remove commented out dev code
d177593 
Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT @nicopap
Construct tips in a smarter way
d1ac848 
Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT
format arrows.rs
e1bde5a
@SIGSTACKFAULT @nicopap
update doc comment on ArrowBuilder.with_tip_length
9bc4cff 
remove "which is sufficient for most applications."

Co-authored-by: Nicola Papale <[email protected]>
@SIGSTACKFAULT
Adjust comments inside ArrowBuilder.drop
a5e68cc
@SIGSTACKFAULT
normalize the tips before multiplying by tip_length
583de5e 
so their lenth is actually `tip_length` and not `sqrt(2)*tip_length`
@IceSentry
Copy link
Contributor

I think it makes sense to keep the head proportional to the arrow length

Yeah, I didn't mean to not keep it proportional. Just limit how big it can get, but that was before I saw that you can specify a size manually so this can just be ignored.

@SIGSTACKFAULT
Copy link
Contributor Author

SIGSTACKFAULT commented Nov 14, 2023 •
edited
Loading

build on macos-latest just says "this job failed" with no explanation or retry button

image

@alice-i-cecile
Copy link
Member

Rerunning CI for you: it's been failing all over the place for no good reason.

@IceSentry
Copy link
Contributor

For consistency, I think I'd prefer keeping arrows inside the gizmos file. If we want to go with separate files per shape I'd prefer if this was done in a separate PR. Not a blocker, just voicing my preference.

@SIGSTACKFAULT
Copy link
Contributor Author

For consistency, I think I'd prefer keeping arrows inside the gizmos file.

I figured the next thing I would do would be to start splitting out the different shapes into their own files

@IceSentry
Copy link
Contributor

Yeah, that's fair, I'd prefer if it was done in a separate PR to make sure people can voice their opinions separately to this PR.

IceSentry
IceSentry approved these changes Nov 14, 2023
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

Like mentioned earlier, I'd like to have the tips connected at the base, but it's not a blocker and can be implemented in a future PR.

@SIGSTACKFAULT
Copy link
Contributor Author

are we waiting for me? or for someone else to hit the merge button

@IceSentry
Copy link
Contributor

IceSentry commented Nov 15, 2023 •
edited
Loading

Bevy PRs require 2 community approvals for a maintainer to then be allowed to do a final review pass and it the merge button.

There's more detail here: https://github.com/bevyengine/bevy/blob/main/CONTRIBUTING.md#making-changes-to-bevy

So we still need another review and a maintainer to hit merge

@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Nov 15, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Nov 15, 2023
Merged via the queue into bevyengine:main with commit ab300d0 Nov 15, 2023
@SIGSTACKFAULT
Copy link
Contributor Author

So we still need another review and a maintainer to hit merge

Just paranoid that y'all were waiting for me and i hadn't realized <3

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
@SIGSTACKFAULT @nicopap
Gizmo Arrows (bevyengine#10550)
183e39c 
## Objective

- Add an arrow gizmo as suggested by bevyengine#9400 

## Solution

(excuse my Protomen music)


https://github.com/bevyengine/bevy/assets/14184826/192adf24-079f-4a4b-a17b-091e892974ec

Wasn't horribly hard when i remembered i can change coordinate systems
whenever I want. Gave them four tips (as suggested by @alice-i-cecile in
discord) instead of trying to decide what direction the tips should
point.

Made the tip length default to 1/10 of the arrow's length, which looked
good enough to me. Hard-coded the angle from the body to the tips to 45
degrees.

## Still TODO

- [x] actual doc comments
- [x] doctests
- [x] `ArrowBuilder.with_tip_length()`

---

## Changelog

- Added `gizmos.arrow()` and `gizmos.arrow_2d()`
- Added arrows to `2d_gizmos` and `3d_gizmos` examples

## Migration Guide

N/A

---------

Co-authored-by: Nicola Papale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile left review comments

@IceSentry IceSentry IceSentry approved these changes

@nicopap nicopap nicopap approved these changes

Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
4 participants
@SIGSTACKFAULT @alice-i-cecile @IceSentry @nicopap