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

Optional ImageScaleMode #11780

Merged
merged 3 commits into from
Feb 9, 2024
Merged

Conversation

ManevilleF
Copy link
Contributor

@ManevilleF ManevilleF commented Feb 8, 2024

Follow up to #11600 and #10588

@mockersf expressed some valid concerns about the current system this PR attempts to fix:

The ComputedTextureSlices reacts to asset change in both bevy_sprite and bevy_ui, meaning that if the ImageScaleMode is inserted by default in the bundles, we will iterate through most 2d items every time an asset is updated.

Solution

  • ImageScaleMode only has two variants: Sliced and Tiled. I removed the Stretched default
  • ImageScaleMode is no longer part of any bundle, but the relevant bundles explain that this additional component can be inserted

This way, the absence of ImageScaleMode means the image will be stretched, and its presence will include the entity to the various slicing systems

Optional components in bundles would make this more straigthfoward

Additional work

Should I add new bundles with the ImageScaleMode component ?

…tched variant

Reduced the computed slices iteration count
@ManevilleF
Copy link
Contributor Author

@mockersf does this adress your concerns ? @alice-i-cecile what do you think of this "extra component" concept ?

@ManevilleF ManevilleF changed the title The image scale mode component is not inserted by default with a Stre… Optional ImageScaleMode Feb 8, 2024
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Feb 8, 2024
Copy link
Contributor

github-actions bot commented Feb 8, 2024

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

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.

I like the additional component: this feels very much like how the ECS architecture is supposed to work. I think you did an excellent job documenting the fact that this exists as well.

@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Feb 8, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I think that's a fair change on this field and can rather improve perf when we don't need this feature, so all good

@pablo-lua
Copy link
Contributor

Should I add new bundles with the ImageScaleMode component?

IMO no, the docs about this feature is good enough, the user can pretty much find this on they own, and we even have examples showing how to use this

@alice-i-cecile alice-i-cecile 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 Feb 9, 2024
@mockersf mockersf added this pull request to the merge queue Feb 9, 2024
Merged via the queue into bevyengine:main with commit e0c296e Feb 9, 2024
28 checks passed
@ManevilleF ManevilleF deleted the chore/clean_slices branch February 9, 2024 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen A-UI Graphical user interfaces, styles, layouts, and widgets C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Performance A change motivated by improving speed, memory usage or compile times 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
Development

Successfully merging this pull request may close these issues.

None yet

4 participants