Rename Core Render Graph Labels#11882
Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom Feb 15, 2024
Merged
Conversation
atlv24
approved these changes
Feb 15, 2024
alice-i-cecile
approved these changes
Feb 15, 2024
IceSentry
approved these changes
Feb 15, 2024
Contributor
IceSentry
left a comment
There was a problem hiding this comment.
I haven't looked at the code but the renames in the description LGTM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
#10644 introduced nice "statically typed" labels that replace the old strings. I would like to propose some changes to the names introduced:
SubGraph2d->Core2dandSubGraph3d->Core3d. The names of these graphs have been / should continue to be the "core 2d" graph not the "sub graph 2d" graph. The crate is calledbevy_core_pipeline, the modules are stillcore_2dandcore_3d, etc.Labels2dandLabels3d, at the very least, should not be plural to follow naming conventions. A Label enum is not a "collection of labels", it is a specific Label. However I thinkLabel2dandLabel3dis significantly less clear thanNode2dandNode3d, so I propose those changes here. I've done the same forLabelsPbr->NodePbrandLabelsUi->NodeUiAdditionally, #10644 accidentally made one of the Camera2dBundle constructors use the 3D graph instead of the 2D graph. I've fixed that here.
Changelog
SubGraph2d->Core2d,SubGraph3d->Core3d,Labels2d->Node2d,Labels3d->Node3d,LabelsUi->NodeUi,LabelsPbr->NodePbr