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

Improve the top docs sections of VFX classes #78865

Merged

Conversation

MewPurPur
Copy link
Contributor

@MewPurPur MewPurPur commented Jun 30, 2023

The top sections of classes are often one of the first things users read about them, so they are important to get right.

Improves the brief descriptions (mainly), main descriptions, and tutorial sections of many VFX-related classes (but not visual shader nodes). ShaderInclude docs was split off into its own thing and already merged.


Unlike my old posts, I've spent some time categorizing these by their importance, in case we want to nitpick.

Simple tweaks for consistency

  • CPUParticles2D, CPUParticles3D, GPUParticles2D, GPUParticles3D, GPUParticlesCollisionBox3D, GPUParticlesCollisionSphere3D: Add the article.
  • GPUParticleAttractor3D, GPUParticlesCollision3D: Use the "Abstract base class" opener. The latter also has a typo fix in its description.
  • ShaderGlobalsOverride, WorldEnvironment: Mention it's a node.
  • FastNoiseLite: Fix a grammatical mistake in the description.
  • Noise: Used [code] tag where appropriate.

Tweaked substantially, changed the flow of ideas

  • CompressedCubemap, CompressedCubemapArray, Cubemap, CubemapArray, Curve, FogMaterial, GPUParticlesAttractorBox3D, GPUParticlesAttractorVectorField3D, Gradient, GradientTexture1D, GradientTexture2D, Material, ORMMaterial3D, PanoramaSkyMaterial, ParticleProcessMaterial, PhysicalSkyMaterial, ShaderMaterial, Sky, StandardMaterial3D, VisualShaderNodeIf, NoiseTexture2D, NoiseTexture3D
  • GPUParticlesAttractorSphere3D: I get why we're using "ellipse-shaped" here even though it's wrong; I think we should be looking at "spheroid".

Doesn't fix a mistake, but still a tangible improvement

  • BackBufferCopy: The brief description had an absurd length, given it appears on places like single-line tooltips. Simplified it significantly and moved details to the main description.
  • BaseMaterial: Mention that it's an abstract base class. Update the title of the tutorial.
  • FogVolume: "Adding local fog" seems misleading, since the node is not just for adding extra fog. Saying that it overrides the global fog properties is more clear in that it can also mean adjusting or reducing the fog.
  • VisualShader: Links to the "Using VisualShaders" tutorial.
  • VisualShaderNode: Updates the title of the tutorial.

Fixes mistakes

  • PlaceholderCubemap, PlaceholderCubemapArray: The description says "texture's dimensions", which is a bit inappropriate as cubemaps are 6-layer textures with equal width and height. It's actually a copy-paste mistake that should be fixed in more "Placeholder" classes.
  • CurveTexture, CurveTextureXYZ: They don't "show a curve", that would imply the curve itself is displayed.
  • GPUParticlesCollisionHeightField3D, GPUParticlesCollisionSDF3D: These classes thought they were attractors.
  • ProceduralSkyMaterial: The description of this class makes reference of a few nonexistent properties and doesn't elaborate enough on DirectionalLight3Ds determining how suns are drawn.

Thanks a ton to Qbie, TMM, and Winston who helped with and workshopped for some of these changes in advance.

@AThousandShips
Copy link
Member

I would suggest separating the changes that fixes clear errors in the documentation from stylistic changes for easier reviewing

@YuriSizov
Copy link
Contributor

cc @QbieShay @hpvb @winston-yallow I would appreciate your reviews of the content since you have been personally named. 🙃

Copy link
Contributor

@QbieShay QbieShay left a comment

Choose a reason for hiding this comment

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

Thanks Mew for the works! Apologies i didn't catch these earlier.

Approval is for content, form to be left to doc team.

doc/classes/ShaderMaterial.xml Outdated Show resolved Hide resolved
doc/classes/ParticleProcessMaterial.xml Outdated Show resolved Hide resolved
doc/classes/ParticleProcessMaterial.xml Outdated Show resolved Hide resolved
doc/classes/GradientTexture2D.xml Outdated Show resolved Hide resolved
doc/classes/FogMaterial.xml Outdated Show resolved Hide resolved
doc/classes/CurveTexture.xml Outdated Show resolved Hide resolved
Copy link
Contributor

@winston-yallow winston-yallow left a comment

Choose a reason for hiding this comment

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

looks good to me 🎉

doc/classes/ShaderGlobalsOverride.xml Outdated Show resolved Hide resolved
@MewPurPur
Copy link
Contributor Author

I would suggest separating the changes that fixes clear errors in the documentation from stylistic changes for easier reviewing

I initially asked Yuri and he suggested I should split PRs based on area. I'll do this extra split if the docs team demands it, and I'll also do it for my following PRs.

I did it like this because I categorized the changes in the post-processing step, after I finished the PR.

@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch from 2f8b117 to 935f127 Compare June 30, 2023 21:24
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I left a few comments and tried to add suggestions wherever I thought an existing description should be improved.

Please let me know if you have any questions about my comments/suggestions!

doc/classes/Cubemap.xml Outdated Show resolved Hide resolved
doc/classes/CurveTexture.xml Outdated Show resolved Hide resolved
doc/classes/CurveTexture.xml Outdated Show resolved Hide resolved
doc/classes/CurveXYZTexture.xml Outdated Show resolved Hide resolved
doc/classes/CurveXYZTexture.xml Outdated Show resolved Hide resolved
doc/classes/GradientTexture2D.xml Outdated Show resolved Hide resolved
doc/classes/ProceduralSkyMaterial.xml Outdated Show resolved Hide resolved
doc/classes/ProceduralSkyMaterial.xml Outdated Show resolved Hide resolved
doc/classes/ShaderGlobalsOverride.xml Outdated Show resolved Hide resolved
doc/classes/ShaderMaterial.xml Outdated Show resolved Hide resolved
@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch 3 times, most recently from f0b76eb to 50f662e Compare July 13, 2023 06:39
doc/classes/ORMMaterial3D.xml Outdated Show resolved Hide resolved
doc/classes/StandardMaterial3D.xml Outdated Show resolved Hide resolved
doc/classes/GPUParticlesAttractorBox3D.xml Outdated Show resolved Hide resolved
doc/classes/GPUParticlesAttractorSphere3D.xml Outdated Show resolved Hide resolved
doc/classes/GPUParticlesAttractorVectorField3D.xml Outdated Show resolved Hide resolved
@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch from 50f662e to 7153e51 Compare July 13, 2023 15:56
@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch from 7153e51 to 8ea2bf7 Compare July 17, 2023 00:41
@MewPurPur
Copy link
Contributor Author

Feedback addressed again :)

@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch 2 times, most recently from 005b173 to 56ae025 Compare August 3, 2023 01:25
@MewPurPur MewPurPur force-pushed the improve-docs-top-sections-VFX branch from 56ae025 to 57d05ff Compare August 11, 2023 18:17
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Amazing work! The current iteration looks good to me. I think we should go ahead and merge this now so it doesn't get stalled forever.

In the future, I highly recommend batching changes into smaller PRs, this size of PR is very hard to review efficiently and thus gets pushed to the backburner for those of us doing reviews

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Aug 16, 2023
@MewPurPur
Copy link
Contributor Author

Duly noted!

@akien-mga akien-mga merged commit 6203f56 into godotengine:master Aug 16, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants