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

Make sprites and text2d work with RenderLayers (add ComputedVisibility) #4007

Closed
wants to merge 4 commits into from

Conversation

johanhelsing
Copy link
Contributor

@johanhelsing johanhelsing commented Feb 21, 2022

Objective

  • Make sprites and text2d work with RenderLayers, so a subset of sprites could be rendered with one camera.

Solution

  • Add ComputedVisibility to SpriteBundle and Text2DBundle
  • Extract the entity handle as well when extracting sprites
  • Create a hash map out of VisibleEntities per view.
  • Check if extracted sprites are present in the hashmap

Maybe there are more efficient ways of doing this than recreating hashmaps all the time?

Anyways, I didn't see any drop in performance when running the bevymark example, so perhaps it's ok.

EDIT: It's a ~17% decline in performance with many_sprites, but no change in bevymark. bevymark is perhaps a bit contrived? Isn't it better to be correct than performant? Most of the performance hit seems to be caused by sampling the hashmap.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 21, 2022
@mockersf
Copy link
Member

Anyways, I didn't see any drop in performance when running the bevymark example, so perhaps it's ok.

Please also check with the many_sprites example

@mockersf mockersf added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Feb 21, 2022
@mcobzarenco
Copy link
Contributor

Maybe there are more efficient ways of doing this than recreating hashmaps all the time?

To avoid the memory allocations, you could store the hash set in a Local and reuse it

@johanhelsing
Copy link
Contributor Author

Please also check with the many_sprites example

Some possible insights into what exactly makes it slower:

  • Only add ComputedVisibility, but don't use it in queue_sprites: 42.5
  • Build the HashSet in queue_sprites, but don't check it: 42

So it seems that building the hash set is not the problem (or at least a very small one), it's actually checking it when iterating the extracted_sprites.

TL;DR: It's a 17% decline in performance with many_sprites, but no change in bevymark. Not sure how contrived many_sprites is? Is it better to be correct or performant?

@superdump
Copy link
Contributor

superdump commented Mar 2, 2022

Did you try iterating the visible entities and getting them from the query and not using a HashSet? I think getting an entity from a query is faster (O(1) maybe?) than checking whether an entity from a query is in a Vec.

@johanhelsing
Copy link
Contributor Author

johanhelsing commented Mar 7, 2022

Did you try iterating the visible entities and getting them from the query and not using a HashSet? I think getting an entity from a query is faster (O(1) maybe?) than checking whether an entity from a query is in a Vec.

Won't this mess up the z-ordering (sprites can be transparent)? I guess I could still try it for comparison. Or do you mean copy all the visible sprites into a new vec and then sort that?

Also, what I'm currently doing is building a hashset (should bo O(n)?), and then checking whether an entity is in that hashset should be average constant time, right?

EDIT: Also, in queue_sprites, ExtractedSprites is a Vec not a Query, so I don't understand how that would work. The sprite query is available in extract_sprites, but at that point I don't think there are any views yet? So I guess in order for this to work, the extracted sprites would have to exist as entities in the render world?

@TethysSvensson
Copy link
Contributor

I would really love to get this merged, as I am using multiple camera with different RenderLayers and would like to be able to put text on only one of them.

TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 5, 2022
Quick and dirty ComputedVisiblity support for sprites, text2d

This is needed for RenderLayers to work properly

Use a HashSet to speed up visibility checking

Remove resolved todo

Don't reallocate visible sprite entities map every frame
TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 5, 2022
Quick and dirty ComputedVisiblity support for sprites, text2d

This is needed for RenderLayers to work properly

Use a HashSet to speed up visibility checking

Remove resolved todo

Don't reallocate visible sprite entities map every frame
TethysSvensson pushed a commit to Skybox-Technologies/bevy that referenced this pull request Apr 19, 2022
Quick and dirty ComputedVisiblity support for sprites, text2d

This is needed for RenderLayers to work properly

Use a HashSet to speed up visibility checking

Remove resolved todo

Don't reallocate visible sprite entities map every frame
@infmagic2047
Copy link
Contributor

I would like to get this merged. #4745 makes it much easier to use multiple cameras, so probably more people will run into this RenderLayers problem.

@johanhelsing
Copy link
Contributor Author

Rebased to fix conflicts

Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

If performance is an issue, I have a small suggestion on how to speed it up.

let extracted_sprites = &mut extracted_sprites.sprites;
let image_bind_groups = &mut *image_bind_groups;

visible_entities_map.clear();
Copy link
Member

@james7132 james7132 Jun 24, 2022

Choose a reason for hiding this comment

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

Consider using a FixedBitset instead of a HashMap here. A contains check using that should be faster than a HashSet, it doesn't involve a O(capacity) scan in the worst case and does not involve a hash on each lookup. Instead of storing the full entity, convert the u32 Entity ID into a usize index, and use that to set and check the FixedBitset.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that that suggestion should be an improvement to performance. I want to try to figure out if there is a way to restructure so that the set can be avoided.

@james7132 did you ever try using a fixed bit set for visible entities? I seem to remember that you did but I don’t remember.

Also, random thought that is not thought through and that may not be good but could be, is it worth storing a fixed bit set of views in which an entity is visible, per entity, instead of a fixed bit set of entities visible per-view? Maybe not as maybe the per-view set is small enough to stay in cache where the per-entity sets would be loaded in as part of iteration… I don’t know.

Copy link
Member

Choose a reason for hiding this comment

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

VisibleEntities IMO shouldn't, though I haven't tried. It'd require an &Entities on the other end to resolve the Entity from the ID, which may or may not be desirable depending on the downstream use case.

Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

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

This will be a useful change if we can reduce/avoid the performance deficit.

events: Res<SpriteAssetEvents>,
mut visible_entities_map: Local<HashSet<Entity>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t a map, it’s a set.

sprite_query: Query<(&Visibility, &Sprite, &GlobalTransform, &Handle<Image>)>,
sprite_query: Query<(
Entity,
&Visibility,
Copy link
Contributor

Choose a reason for hiding this comment

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

ComputedVisibility needs to be used for extraction.

atlas_query: Query<(
Entity,
&Visibility,
Copy link
Contributor

Choose a reason for hiding this comment

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

ComputedVisibility needs to be used for extraction here too.

let extracted_sprites = &mut extracted_sprites.sprites;
let image_bind_groups = &mut *image_bind_groups;

visible_entities_map.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that that suggestion should be an improvement to performance. I want to try to figure out if there is a way to restructure so that the set can be avoided.

@james7132 did you ever try using a fixed bit set for visible entities? I seem to remember that you did but I don’t remember.

Also, random thought that is not thought through and that may not be good but could be, is it worth storing a fixed bit set of views in which an entity is visible, per entity, instead of a fixed bit set of entities visible per-view? Maybe not as maybe the per-view set is small enough to stay in cache where the per-entity sets would be loaded in as part of iteration… I don’t know.

@infmagic2047
Copy link
Contributor

In addition to SpriteBundle, a ComputedVisibility component is needed on SpriteSheetBundle as well.

@cart cart added this to the Bevy 0.8 milestone Jun 26, 2022
@cart
Copy link
Member

cart commented Jun 26, 2022

Given that RenderLayers-like functionality is pretty important for things like "camera driven rendering", I think we might need to just eat this perf cost.

Added this to the 0.8 milestone as I think its worth making a call on "2d visibility controls" as part of the "camera driven rendering" story.

@superdump
Copy link
Contributor

This was last updated 24 days ago but #4489 was merged 12 days ago and will impact real world performance. Also, I have ideas for improving sprite rendering performance further so we can probably take the hit.

@kserz
Copy link

kserz commented Jun 27, 2022

I've made a version #5114 with a different tradeoff between straight logic and performance. Any opinions are much appreciated.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.8 milestone Jun 27, 2022
@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jun 27, 2022
@TethysSvensson
Copy link
Contributor

Has this been superseeded by #5310?

@superdump
Copy link
Contributor

Yes, Sprites and Text2d now support RenderLayers and ComputedVisibility

@superdump superdump closed this Jul 19, 2022
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 C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants