-
Notifications
You must be signed in to change notification settings - Fork 15
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
GFX & Shader Documentation #472
Conversation
a03cf67
to
205884d
Compare
Opening it now for review since it might get too big. Since this also contains a bunch of technical fixes/changes. I still plan to continue documenting the rest of the graphics shards, add more samples & add more details on some topics. |
Looks good! Going forward we should be trying to standardize the writing of docs for Shards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are too many changes that are unrelated with each others. Some may be based on misconception on how the documentation is generated.
The list of samples in generated doc is broken: the
was required to separate groups of Code/Output.
help ()shouldn't have any markdown or HTML (or even line return) in it as it will be used by the visual scripting and it is undecided at this stage if we will support markdown there (or another syntax). That's why we have the Details section, where more "complex" documentation can be manually written.
Please split into several PRs, starting with all the changes related to GFX code being separate, then every new doc feature being also separate so we can select the one we want.
- PR related to search
- PR related to reformatting the input/output/parameter into a single table (for which I don't see why we need HTML, as markdown is capable of doing tables)
Also at this stage, I wouldn't move any pages around since this is still a work in progress. That should be part of another PR and ongoing discussion.
There are good changes like the generation of enum pages. But I'd rather we review those separately.
Rebased & cleaned up |
Codecov Report
@@ Coverage Diff @@
## devel #472 +/- ##
==========================================
- Coverage 81.22% 81.22% -0.01%
==========================================
Files 219 219
Lines 29183 29183
==========================================
- Hits 23705 23703 -2
- Misses 5478 5480 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Just a remark regarding documenting the enums.
ENUM_HELP(gfx::BuiltinFeatureId, gfx::BuiltinFeatureId::Transform, SHCCSTR("Add basic world/view/projection transform")); | ||
ENUM_HELP(gfx::BuiltinFeatureId, gfx::BuiltinFeatureId::BaseColor, | ||
SHCCSTR("Add basic color from vertex color and (optional) color texture")); | ||
ENUM_HELP(gfx::BuiltinFeatureId, gfx::BuiltinFeatureId::VertexColorFromNormal, SHCCSTR("Outputs color from vertex color")); | ||
ENUM_HELP(gfx::BuiltinFeatureId, gfx::BuiltinFeatureId::Wireframe, SHCCSTR("Modifies the main color to visualize vertex edges")); | ||
ENUM_HELP(gfx::BuiltinFeatureId, gfx::BuiltinFeatureId::Velocity, | ||
SHCCSTR("Outputs object velocity into the velocity global & output")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChelseaChX Should we have a final .
for those help strings? I added one on my own enum description since we also had those on the regular help()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think we have them everywhere else in help strings, can add them here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChelseaChX Should we have a final
.
for those help strings? I added one on my own enum description since we also had those on the regularhelp()
function.
Yeah we should keep it consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good as a starting point I guess. But needs way more love.
@sinkingsugar Yeah not complete yet, but I'll open a new PR for the remaining docs |
Preview
Document GFX shards: