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

Type description improvements #2176

Open
wants to merge 29 commits into
base: main
Choose a base branch
from

Conversation

niklasharrysson
Copy link
Contributor

This change list is an overhaul of the type system in shader generation.

Improvements include:

  • Removing the singleton pattern for TypeDesc storage and access, making the type system thread safe.
  • Improved handling of TypeDesc internal data, supporting any amount of data for the future, while keeping the class size minimal.
  • Handling of re-registration of type descriptions.
  • Support for keeping type descriptions alive after the lifetime of ShaderGenerator and GenContext. For applications that holds/uses the ShadePort reflection data that is generated by shader generation, even after the ShaderGenerator itself is destroyed. (To do this an application can create and pass in an external TypeSystem).

Co-work with @ld-kerley

@niklasharrysson
Copy link
Contributor Author

This PR includes and replaces #2107, which can be closed if this PR is approved.

/// Optionally pass in an externally created TypeSystem here,
/// if you want to keep type descriptions alive after the lifetime
/// of the shader generator.
EsslShaderGenerator(TypeSystemPtr typeSystem = TypeSystem::create());
Copy link
Member

Choose a reason for hiding this comment

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

@niklasharrysson @ld-kerley Since we're encouraging developers to use the associated create function for each ShaderGenerator subclass, I would recommend omitting default arguments for our ShaderGenerator constructors, leaving only the default arguments on our ShaderGenerator create functions. This would simplify the code a bit, and would help to clarify which entry point developers are expected to use.

Does that sound like a reasonable simplification to you both?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! I’ll make that change 👍

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

Successfully merging this pull request may close these issues.

2 participants