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 rendering components for improved consistency and clarity #15035

Merged
merged 11 commits into from
Sep 10, 2024

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Sep 3, 2024

Objective

The names of numerous rendering components in Bevy are inconsistent and a bit confusing. Relevant names include:

  • AutoExposureSettings
  • AutoExposureSettingsUniform
  • BloomSettings
  • BloomUniform (no Settings)
  • BloomPrefilterSettings
  • ChromaticAberration (no Settings)
  • ContrastAdaptiveSharpeningSettings
  • DepthOfFieldSettings
  • DepthOfFieldUniform (no Settings)
  • FogSettings
  • SmaaSettings, Fxaa, TemporalAntiAliasSettings (really inconsistent??)
  • ScreenSpaceAmbientOcclusionSettings
  • ScreenSpaceReflectionsSettings
  • VolumetricFogSettings

Firstly, there's a lot of inconsistency between Foo/FooSettings and FooUniform/FooSettingsUniform and whether names are abbreviated or not.

Secondly, the Settings post-fix seems unnecessary and a bit confusing semantically, since it makes it seem like the component is mostly just auxiliary configuration instead of the core thing that actually enables the feature. This will be an even bigger problem once bundles like TemporalAntiAliasBundle are deprecated in favor of required components, as users will expect a component named TemporalAntiAlias (or similar), not TemporalAntiAliasSettings.

Solution

Drop the Settings post-fix from the component names, and change some names to be more consistent.

  • AutoExposure
  • AutoExposureUniform
  • Bloom
  • BloomUniform
  • BloomPrefilter
  • ChromaticAberration
  • ContrastAdaptiveSharpening
  • DepthOfField
  • DepthOfFieldUniform
  • DistanceFog
  • Smaa, Fxaa, TemporalAntiAliasing (note: we might want to change to Taa, see "Discussion")
  • ScreenSpaceAmbientOcclusion
  • ScreenSpaceReflections
  • VolumetricFog

I kept the old names as deprecated type aliases to make migration a bit less painful for users. We should remove them after the next release. (And let me know if I should just... not add them at all)

I also added some very basic docs for a few types where they were missing, like on Fxaa and DepthOfField.

Discussion

  • TemporalAntiAliasing is still inconsistent with Smaa and Fxaa. Consensus on Discord seemed to be that renaming to Taa would probably be fine, but I think it's a bit more controversial, and it would've required renaming a lot of related types like TemporalAntiAliasNode, TemporalAntiAliasBundle, and TemporalAntiAliasPlugin, so I think it's better to leave to a follow-up.
  • I think Fog should probably have a more specific name like DistanceFog considering it seems to be distinct from VolumetricFog. This should probably be done in a follow-up though, so I just removed the Settings post-fix for now. (done)

Migration Guide

Many rendering components have been renamed for improved consistency and clarity.

  • AutoExposureSettingsAutoExposure
  • BloomSettingsBloom
  • BloomPrefilterSettingsBloomPrefilter
  • ContrastAdaptiveSharpeningSettingsContrastAdaptiveSharpening
  • DepthOfFieldSettingsDepthOfField
  • FogSettingsDistanceFog
  • SmaaSettingsSmaa
  • TemporalAntiAliasSettingsTemporalAntiAliasing
  • ScreenSpaceAmbientOcclusionSettingsScreenSpaceAmbientOcclusion
  • ScreenSpaceReflectionsSettingsScreenSpaceReflections
  • VolumetricFogSettingsVolumetricFog

@Jondolf Jondolf added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 3, 2024
@alice-i-cecile alice-i-cecile added X-Contentious There are nontrivial implications that should be thought through M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 3, 2024
@alice-i-cecile
Copy link
Member

I think Fog should probably have a more specific name like DistanceFog considering it seems to be distinct from VolumetricFog. This should probably be done in a follow-up though, so I just removed the Settings post-fix for now.

Agreed!

@JMS55
Copy link
Contributor

JMS55 commented Sep 3, 2024

Imo we should do the fog -> distance fog in this PR.

I'd also like if you could rename variables and bundle fields to remove "settings".

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 3, 2024

Imo we should do the fog -> distance fog in this PR.

Done!

I'd also like if you could rename variables and bundle fields to remove "settings".

This feels like it's a bit out of scope for this PR, but I renamed a bunch of variables to remove _settings and overall tried to make names clearer where it made sense. Also added some basic docs that were missing.

I don't think we want to do this for bundles though. The impression I got is that we don't want to cause any unnecessary breaking changes for them (like remove or rename properties), considering the plan is to deprecate (most of) the first-party bundles in favor of required components in 0.15.

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 3, 2024

Done!

Hmm or I guess for fog I should also rename GpuFog, FogMeta, and maybe some shader stuff to have the Distance prefix as well? I'm not familiar with the fog rendering at all, so I don't really know if any of it is shared between the distance fog and volumetric fog. To me it looks like basically all of the general Fog and GpuFog stuff is only for distance fog and atmospheric fog, but I'm not 100% sure, which is a part of why I would've liked to leave that stuff to a follow-up / someone who knows what all the types actually do 😅

@alice-i-cecile
Copy link
Member

Agreed on leaving the bundles alone. I'm fine to leave the fog stuff alone at this point: I like the initial rename to avoid a double-rename, making it harder to compile the migration guide.

@MrGVSV
Copy link
Member

MrGVSV commented Sep 4, 2024

TemporalAntiAliasing is still inconsistent with Smaa and Fxaa. Consensus on Discord seemed to be that renaming to Taa would probably be fine, but I think it's a bit more controversial, and it would've required renaming a lot of related types like TemporalAntiAliasNode, TemporalAntiAliasBundle, and TemporalAntiAliasPlugin, so I think it's better to leave to a follow-up.

I have no knowledge on rendering or what any of this stuff does haha but what about expanding Smaa and Fxaa?

If I know I need "anti-aliasing", I imagine it would be really helpful to just type that in on the docs to find all my options. You could probably achieve that with a doc alias though but I don't think that helps when doing the same sort of thing in my IDE and letting intellisense be my quasi-doc-search.

Although, I imagine SubpixelMorphologicalAntiAliasingNeighborhoodBlendingPipelineKey is not the friendliest of identifiers lol.

You could maybe do it for the "main component" but I guess that could be seen as adding more inconsistencies.

In either case (not necessarily part of this PR), the docs for these acronyms should at least start out with what they stand for. SmaaSettings puts it at the end of the sentence and Fxaa doesn't seem to have any.

@Jondolf
Copy link
Contributor Author

Jondolf commented Sep 4, 2024

In either case (not necessarily part of this PR), the docs for these acronyms should at least start out with what they stand for. SmaaSettings puts it at the end of the sentence and Fxaa doesn't seem to have any.

I actually added basic docs for these in this PR already, along with adding the long names as doc aliases (even though it's technically not related to this PR). See Fxaa and Smaa in the diff :)

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Much nicer!

@cart cart enabled auto-merge September 10, 2024 01:03
@cart cart added this pull request to the merge queue Sep 10, 2024
@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 Sep 10, 2024
Merged via the queue into bevyengine:main with commit afbbbd7 Sep 10, 2024
27 checks passed
@Jondolf Jondolf deleted the rendering-renames branch September 10, 2024 08:12
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-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
Status: Responded
Development

Successfully merging this pull request may close these issues.

5 participants