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

[Merged by Bors] - Remove Need for Sprite Size Sync System #2632

Closed

Conversation

zicklag
Copy link
Member

@zicklag zicklag commented Aug 10, 2021

Objective

  • Prevent the need to have a system that synchronizes sprite sizes with their images

Solution

  • Read the sprite size from the image asset when rendering the sprite
  • Replace the size and resize_mode fields of Sprite with a custom_size: Option<Vec2> that will modify the sprite's rendered size to be different than the image size, but only if it is Some(Vec2)

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed C-Feature A new feature, making something new possible labels Aug 10, 2021
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This is a much better approach, and very simply implemented. Nice work!

@cart
Copy link
Member

cart commented Aug 13, 2021

This is definitely reasonable, but it creates some new problems: users will almost certainly want to query the current sprite size in userspace. Things like culling, bounding boxes, etc will also need per-sprite sizes. Users can manually read the texture size, but this involves more boilerplate than most people will want.

@zicklag
Copy link
Member Author

zicklag commented Aug 13, 2021

Ah, I see, that makes sense. 🤔

Maybe this doesn't help much then. I'll think a bit more about the problem to see if I can come up with any other ways to improve the situation.

@zicklag
Copy link
Member Author

zicklag commented Aug 19, 2021

OK, what do you think of this:

  • We keep this PR's changes of removing the resize_mode and size fields and replacing it with a custom_size field
  • We also add new field called something like computed_size, calculated_size, or actual_size and we use a system to update that field with the size calculated from the texture, unless there is a custom_size, in which case it just sets it to the custom size

I think that might just make the situation a little more clear. I had always been confused by the resize mode thing until I looked at the implementation. That could easily be solved by documentation, but I think the solution above is more self explanatory.

@cart
Copy link
Member

cart commented Aug 19, 2021

Alrighty I've thought about this for a bit and I think I'm actually in favor of merging the current implementation. I don't want to store "computed size" on the Sprite itself. It should probably live on something like a BoundingRect component. There is also the issue of rect size needing to be in sync with the transform scale, which complicates things even more. I'd rather just tackle that separately (when it is needed).

Short term I think adopting the cheaper / simpler approach is the way to go. Users that need the actual width and height can look up the texture dimensions and scale it using the transform. This approach will be forward-compatible with whatever approach we adopt for bounding rects.

@cart
Copy link
Member

cart commented Aug 25, 2021

@zicklag can you resolve merge conflicts? I did this locally but I cant push it because you've disallowed that.

Replace the `Sprite::size` field with an optional `Sprite::custom_size`
field and use the sprite image size for rendering unless `custom_size`
is specified.
@zicklag
Copy link
Member Author

zicklag commented Aug 26, 2021

I did this locally but I cant push it because you've disallowed that.

Ah, yeah, apparently that's what GitHub does when you use an organization account to fork. I wonder if they let you chnage that. 🤔 I think I can just start pushing to my zicklag fork instead.

Anyway, pushed an update!

@cart
Copy link
Member

cart commented Aug 26, 2021

Ah thats good to know. Thanks!

@cart
Copy link
Member

cart commented Aug 26, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 26, 2021
# Objective

- Prevent the need to have a system that synchronizes sprite sizes with their images

## Solution

- Read the sprite size from the image asset when rendering the sprite
- Replace the `size` and `resize_mode` fields of `Sprite` with a `custom_size: Option<Vec2>` that will modify the sprite's rendered size to be different than the image size, but only if it is `Some(Vec2)`
@bors
Copy link
Contributor

bors bot commented Aug 26, 2021

Build failed:

@cart
Copy link
Member

cart commented Aug 26, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 26, 2021
# Objective

- Prevent the need to have a system that synchronizes sprite sizes with their images

## Solution

- Read the sprite size from the image asset when rendering the sprite
- Replace the `size` and `resize_mode` fields of `Sprite` with a `custom_size: Option<Vec2>` that will modify the sprite's rendered size to be different than the image size, but only if it is `Some(Vec2)`
@bors
Copy link
Contributor

bors bot commented Aug 26, 2021

@bors bors bot changed the title Remove Need for Sprite Size Sync System [Merged by Bors] - Remove Need for Sprite Size Sync System Aug 26, 2021
@bors bors bot closed this Aug 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants