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

Picking: Declare working space in HitData position #17844

Closed
bytemunch opened this issue Feb 13, 2025 · 3 comments · Fixed by #17859
Closed

Picking: Declare working space in HitData position #17844

bytemunch opened this issue Feb 13, 2025 · 3 comments · Fixed by #17859
Labels
A-Picking Pointing at and selecting objects of all sorts C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@bytemunch
Copy link
Contributor

What problem does this solve or what need does it fill?

Currently the returned position field in HitData is space-ambiguous, leading users down a docs rabbit hole as they need to discern what backend is being used, then access that backend's docs to learn what space the position data is in.

What solution would you like?

Something similar to Projection's variant solution, like

pub enum PositionSpace {
    World(Vec3),
    Local(Vec3),
    Screen(Vec2),
    Ui(Vec3),
    // ...etc.
}

This could extend to a better all-round solution, perhaps allowing normal data to be only included where necessary, e.g.

pub enum PickPosition {
    World {
        position: PositionSpace,
        normal: PositionSpace,
    },
    Screen(Vec2),
    Ui(Vec3),
    // ...etc.
}

(of course this is a rough outline, above code snippets are nonsense together but I hope you get the idea :) )

What alternative(s) have you considered?

  • Leave things as-is, expect user diligence
  • Enforce specific spaces per dimension, all 3d in world space, all 2d in screen space

Additional context

Inspired by @notmd's musings on the subject here

Related: #17681

@bytemunch bytemunch added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 13, 2025
@notmd
Copy link
Contributor

notmd commented Feb 13, 2025

This will make the HitData size larger and significantly degrade DX because you have to match each variant, you already have to handle the Option. I think adding docs which space is used to each backend is enough.

@bytemunch
Copy link
Contributor Author

Oh I thought enums had compiler magic making them size free, research shows they're max variant size + 8 bytes, making this a bad solution for something done multiple times a frame. The matching issue is a pain too, I guess a user would need to know in advance what they're destructuring to, meaning the doc trail still needs to be followed.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! A-Picking Pointing at and selecting objects of all sorts and removed C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Feb 13, 2025
@alice-i-cecile
Copy link
Member

I'd prefer docs here :) If we really want to enforce this we could do an associated constant for each backend or something but let's start with a light touch.

github-merge-queue bot pushed a commit that referenced this issue Feb 15, 2025
# Objective

Add reference to reported position space in picking backend docs.

Fixes #17844 

## Solution

Add explanatory docs to the implementation notes of each picking
backend.

## Testing

`cargo r -p ci -- doc-check` & `cargo r -p ci -- lints`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Picking Pointing at and selecting objects of all sorts C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants