-
Notifications
You must be signed in to change notification settings - Fork 55
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
refactor: simplify feature implementation #326
Conversation
fixes #186 |
8b62f79
to
2844da4
Compare
This needs to be updated to support other PRs I believe. |
aba4b45
to
d44d5d4
Compare
@doodlum Updated to latest dev now! |
|
||
virtual void DrawSettings() = 0; | ||
virtual void Draw(const RE::BSShader* shader, const uint32_t descriptor) = 0; | ||
virtual void DrawSettings() {} | ||
virtual void DrawDeferred() {} |
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.
DrawDeferred
and DrawPreProcess
can be removed, too, since they are not referenced.
On a side note, some static analysis tools that find dead code would be convenient
@@ -91,22 +91,15 @@ struct DynamicCubemaps : Feature | |||
|
|||
virtual inline std::string GetName() { return "Dynamic Cubemaps"; } |
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.
Why are GetName
and GetShortName
not marked override
?
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.
I decided to not have the override due to them being pure virtual so not really "overring" another "empty" implementation. But maybe it's best practice to mark that with override as well? Not sure
} | ||
|
||
void ScreenSpaceGI::Save([[maybe_unused]] json& o_json) | ||
void ScreenSpaceGI::SaveSettings([[maybe_unused]] json& o_json) |
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.
This is mostly my fault but you can delete [[maybe_unused]]
here.
Refactor:
Created it as as a draft due to some more testing needed before publishing itTested now, works like before