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

Rename "point light" to "clusterable object" in cluster contexts. #13654

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Jun 3, 2024

We want to use the clustering infrastructure for light probes and decals as well, not just point lights. This patch builds on top of #13640 and performs the rename.

To make this series easier to review, this patch makes no code changes. Only identifiers and comments are modified.

Migration Guide

  • In the PBR shaders, point_lights is now known as clusterable_objects, PointLight is now known as ClusterableObject, and cluster_light_index_lists is now known as clusterable_object_index_lists.

We want to use the clustering infrastructure for light probes and decals
as well, not just point lights. This patch builds on top of bevyengine#13640 and
performs the rename.

To make this series easier to review, this patch makes no code changes.
Only identifiers and comments are modified.
@BD103 BD103 added A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide X-Contentious There are nontrivial implications that should be thought through D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 3, 2024
@BD103
Copy link
Member

BD103 commented Jun 3, 2024

I added the contentious label because it's a non-trivial breaking change that may affect the ecosystem as a whole. If the rendering people generally agree with this change, it can be switch to X-Blessed.

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.

Diff looks fine to me: this really is just a rename. I like the move from light -> "anything that could be clustered". I'd be fine with dropping _object from clusterable_object, but e.g. y_object really needs a noun to go with it and should stay in its current form.

@IceSentry
Copy link
Contributor

IceSentry commented Jun 3, 2024

I haven't discussed it with other SMEs yet, but I don't think this is contentious. It's pretty much agreed that this code needs a refactor and the change is motivated by future features that we also want.

@BD103 I'm also not sure why you consider it non-trivial, it's just a rename with no behaviour change and the things that are renamed are probably not used by the vast majority of users except advanced users making custom lighting systems. A migration guide entry mentioning the rename seems more than enough to me.

@alice-i-cecile alice-i-cecile added X-Uncontroversial This work is generally agreed upon and removed X-Contentious There are nontrivial implications that should be thought through labels Jun 3, 2024
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I tested a bunch of examples just to make sure nothing was missed and didn't find any regressions.

A note about rust side renames in the migration guide section would be nice. Doesn't necessarily have to be the entire list but at least mention it's not just shader stuff that's been renamed.

LGTM assuming CI is fixed (I think it's just fmt stuff)

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 3, 2024
@alice-i-cecile
Copy link
Member

Once CI is passing I'll merge this in.

@BD103
Copy link
Member

BD103 commented Jun 3, 2024

@BD103 I'm also not sure why you consider it non-trivial, it's just a rename with no behaviour change and the things that are renamed are probably not used by the vast majority of users except advanced users making custom lighting systems. A migration guide entry mentioning the rename seems more than enough to me.

I mainly marked it as straightforward due to the amount of files being modified, even if those modifications are trivial. It's really up to interpretation, so I'll switch it over. :)

@BD103 BD103 added D-Trivial Nice and easy! A great choice to get started with Bevy and removed D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jun 3, 2024
@IceSentry
Copy link
Contributor

@BD103 Ah, I didn't realize that's what you meant by non-trivial, I wasn't thinking about labels, I think the "may affect the ecosystem" part was what confused me. I agree that the straightforward label is fine here.

@BD103
Copy link
Member

BD103 commented Jun 4, 2024

@BD103 Ah, I didn't realize that's what you meant by non-trivial, I wasn't thinking about labels, I think the "may affect the ecosystem" part was what confused me. I agree that the straightforward label is fine here.

Yup! It was probably confusing because my understanding of Bevy's rendering setup is severely lacking. You didn't understand it because I probably didn't understand it either; I just guessed that a rename may affect existing libraries / plugins.

Either way, glad we got this resolved!

@BD103 BD103 added D-Straightforward Simple bug fixes and API improvements, docs, test and examples and removed D-Trivial Nice and easy! A great choice to get started with Bevy labels Jun 4, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 4, 2024
Merged via the queue into bevyengine:main with commit ad68722 Jun 4, 2024
27 checks passed
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants