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

Bind point light counts #25

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Bind point light counts #25

merged 3 commits into from
Aug 20, 2024

Conversation

jgayfer
Copy link
Owner

@jgayfer jgayfer commented Aug 18, 2024

Summary

Once space is allocated in a buffer, it's allocated. Despawning a light doesn't clear the memory in the binding, unless we make a new binding.

Prior to this change, despawning a light could have it hang around, still in memory.

This change adds a separate binding to track the number of active point lights, and uses the number to iterate through the point light buffer, ensuring we won't over step.

Fixes #18

Todo

  • Test WebGPU
  • Test WebGL2

@jgayfer
Copy link
Owner Author

jgayfer commented Aug 18, 2024

@drawk-cab I need to do some more testing, but wouldn't mind a sanity check from you here.

I'm fairly certain it fixes the example you provided, but never hurts to double check.

@jgayfer jgayfer force-pushed the point-light-count branch 2 times, most recently from 6c4f0a8 to d431ee1 Compare August 19, 2024 00:40
@jgayfer jgayfer marked this pull request as ready for review August 19, 2024 05:11
Copy link
Contributor

@isidornygren isidornygren left a comment

Choose a reason for hiding this comment

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

Nice job! I hope you don't mind me checking this out, not sure what process you prefer when it comes to reviews so I just went for it. I found a tiny thing which tbh I'm not terribly sure how much time it would actually save, but I noticed that bevy uses len instead of count internally.

I tested this branch using the example provided in the issue: #18 but with 100 entities instead of one and all the lights correctly despawned (I double checked that running the example on main caused the lights not to despawn). So I think this is good to go.

src/render/light_map/prepare.rs Outdated Show resolved Hide resolved
@jgayfer jgayfer force-pushed the point-light-count branch from d431ee1 to 7622208 Compare August 20, 2024 03:36
Once space is allocated in a buffer, it's allocated. Despawning a light
doesn't clear the memory in the binding, unless we make a new binding.

Prior to this change, despawning a light could have it hang around, still
in memory.

This change adds a separate binding to track the number of active point
lights, and uses the number to iterate through the point light buffer,
ensuring we won't over step.
@jgayfer jgayfer force-pushed the point-light-count branch from 7622208 to e66ad93 Compare August 20, 2024 03:40
@jgayfer
Copy link
Owner Author

jgayfer commented Aug 20, 2024

Nice job! I hope you don't mind me checking this out, not sure what process you prefer when it comes to reviews so I just went for it.

@isidornygren My strategy for reviews is usually to put up a PR and end up admin merging it 😅 I'm more than happy to have some feedback on changes. Please review as much as you'd like! A second pair of eyes is always helpful.

Thanks for pitching in (especially for pulling it down for a quick test).

@jgayfer jgayfer merged commit b17d245 into main Aug 20, 2024
6 checks passed
@jgayfer jgayfer deleted the point-light-count branch October 29, 2024 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Light doesn't go away when its entity is despawned
2 participants