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

Shader generators are converted to modules #5552

Merged
merged 2 commits into from Aug 17, 2023
Merged

Conversation

mvaligursky
Copy link
Contributor

@mvaligursky mvaligursky commented Aug 4, 2023

  • new base class for shader generators, called ShaderGenerator, implements functionality from now deleted common.js as static functions (so that they can be called from outside of generators too)
  • all shader generator (basic, standard, particle, sky, pass-through) extend this class, and generate and export a single instance of it

@mvaligursky mvaligursky self-assigned this Aug 4, 2023
@mvaligursky mvaligursky added the area: graphics Graphics related issue label Aug 4, 2023
const particle = {
generateKey: function (options) {
class ShaderGeneratorParticle extends ShaderGenerator {
generateKey(options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these generateKey functions be static? If all functions on these classes were static, no need to create instances. Just export the classes directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

standard.js contains some members as well .. I'm not sure I'd want to change those and all functions in it to static, instances seems more flexible for the future too.


const basic = {
generateKey: function (options) {
class ShaderGeneratorBasic extends ShaderGenerator {
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this extends?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To give all those classes base class / shared type information.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see that used anywhere?

The structure of this whole class hierarchy is very non-standard (and for me confusing).

ShaderGenerator isn't a real base class because it defines no base functionality. Rather it is a set of static helper functions.

Then ShaderGeneratorBasic and the rest implement what looks like a consistent set of functions (which presumably program-libraray expects, but never defines).

Ultimately the structure/interface of ShaderGenerator isn't defined anywhere and is left as implicit.

I don't think there needs to be any inheritance here and probably program-library should define what structure a 'generator' takes. Each implementation of generator just implements that interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point I'm going to add type information to the program-library, and this is the type of the generator. Ideally this would be an interface, but unfortunately JS does not have a concept of this. But this is the only way to assign a single (base) type to multiple classes.

Copy link
Member

Choose a reason for hiding this comment

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

If program-library just uses duck typing on generators, why do generators need a class hierarchy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added some JSDocs to the program-libray that use the base class type - type safety is where we're going to with the engine changes.

Copy link
Member

@slimbuck slimbuck left a comment

Choose a reason for hiding this comment

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

Approving under the assumption this mess is cleaned up in future :)

@mvaligursky mvaligursky merged commit ed5dbcf into main Aug 17, 2023
7 checks passed
@mvaligursky mvaligursky deleted the mv-shader-generator branch August 17, 2023 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: graphics Graphics related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants