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

Add a ShaderGen option to select the target MDL version #1417

Conversation

krohmerNV
Copy link
Contributor

This allows to generate MDL code for older applications while adding new features in the future.

It also to helps structuring changes and features depending on the MDL version.

Work in progress because i would like have feedback first.

@jstone-lucasfilm
Copy link
Member

This looks like a really promising improvement, @krohmerNV, and let us know when you feel it's ready for review.

@krohmerNV
Copy link
Contributor Author

@jstone-lucasfilm thanks! I'm working on a few test on our side to check if the generated code really works with old versions of our SDK. I don't plan to add major changes anymore. So, if you find time to review, I would very much appreciate it already.

@jstone-lucasfilm
Copy link
Member

The overall structure of this proposal looks good to me, @krohmerNV, and I'm CC'ing @niklasharrysson for his thoughts and expertise.

@niklasharrysson
Copy link
Contributor

Sorry for the delay, I noticed this ping today.

This looks good to me as well @krohmerNV , and I only had a few minor comments above.

@krohmerNV
Copy link
Contributor Author

@niklasharrysson thanks for the review.
i missed it as well, sorry.
will try to update today. maybe it's good for 1.38.9

@jstone-lucasfilm
Copy link
Member

Thanks @krohmerNV, and let's definitely aim to include this change in 1.38.9 if you have the bandwidth to update it.

@krohmerNV krohmerNV changed the title WIP: Add an MDL ShaderGen option to select the target MDL version Add an MDL ShaderGen option to select the target MDL version Feb 1, 2024
@krohmerNV
Copy link
Contributor Author

My local tests look fine. Ready from my side.

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks great to me, thanks @krohmerNV!

@jstone-lucasfilm jstone-lucasfilm changed the title Add an MDL ShaderGen option to select the target MDL version Add a ShaderGen option to select the target MDL version Feb 5, 2024
@jstone-lucasfilm jstone-lucasfilm merged commit de5c2b6 into AcademySoftwareFoundation:main Feb 5, 2024
31 checks passed
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.

3 participants