Skip to content

Arrow Head Generalization #10661

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

Closed

Conversation

SIGSTACKFAULT
Copy link
Contributor

@SIGSTACKFAULT SIGSTACKFAULT commented Nov 20, 2023

Objective

  • Generalize Arrow Heads
  • Multiple head types
  • Different head types on each end

Solution

  • WIP

Changelog

  • WIP

Migration Guide

  • WIP

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Gizmos Visual editor and debug gizmos labels Nov 20, 2023
/// }
/// # bevy_ecs::system::assert_is_system(system);
/// ```
pub fn with_double_ended(&mut self, state: bool) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not split this into with_double_ended and with_single_ended, and remove the bool arg?

Copy link
Contributor Author

@SIGSTACKFAULT SIGSTACKFAULT Nov 20, 2023

Choose a reason for hiding this comment

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

I didn't see any parcticular reason to do it as one or two functions so i just picked one because it was less typing /shrug

either way is fine by me.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think that two functions will be nicer to use for end users because in 99% of cases you'll just call this once to toggle double-ended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Idk but with_.._ended sounds weird to me. What about this:

gizmos.arrow().with_ending(ArrowEnding::Single);
gizmos.arrow().with_ending(ArrowEnding::Double)

?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if we later want multiple styles of ending:

gizmos.arrow().with_endings((ArrowEnding::None, ArrowEnding::Normal));

Copy link
Member

Choose a reason for hiding this comment

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

gizmos.arrow().with_tips(ArrowTip::None, ArrowTip::Normal);

Gets my vote.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gizmos.arrow().with_endings((ArrowEnding::None, ArrowEnding::Normal));

I like this idea but I think there should still be single_ended and double_ended methods for simpler use-cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scope creep time!

Copy link
Contributor

Choose a reason for hiding this comment

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

These cover all 4 possible combinations of tips for almost all use cases:

gizmos.line()
gizmos.arrow()
gizmos.double_arrow()

There's no need for additional flexibility in debug gizmos... but you can do this:

gizmos.double_arrow()
    .with_front_tip(ArrowTip {
        length: 10.,
        color: Color::WHITE,
        style: ArrowStyle::Kitten,
        ..default()
    })

Copy link
Contributor Author

@SIGSTACKFAULT SIGSTACKFAULT Nov 20, 2023

Choose a reason for hiding this comment

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

yeah. the more i write down the scope-crept version in my head the more i realize it's a bad idea.

@SIGSTACKFAULT
Copy link
Contributor Author

SIGSTACKFAULT commented Nov 20, 2023

my adhd knows no boundries. scope creep time.

@SIGSTACKFAULT SIGSTACKFAULT marked this pull request as draft November 20, 2023 19:32
@SIGSTACKFAULT SIGSTACKFAULT changed the title Add Double-Ended Arrows Arrow Head Generalization Nov 20, 2023
@SIGSTACKFAULT
Copy link
Contributor Author

SIGSTACKFAULT commented Nov 22, 2023

I think my first idea (maybe with shorter method names on ArrowBuilder) was better, or just adding gizmos.double_arrow()

@SIGSTACKFAULT
Copy link
Contributor Author

if you really want a double-headed arrow just do a second arrow pointing the other way tbh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Gizmos Visual editor and debug gizmos C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants