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 combined version define in MaterialX C++ #2031

Conversation

ruiyang2021
Copy link
Contributor

@ruiyang2021 ruiyang2021 commented Sep 27, 2024

PR for #1609

Also added a simple test in source/MaterialXTest/MaterialXCore/Library.cpp. Let me know if that's unnecessary

Copy link

linux-foundation-easycla bot commented Sep 27, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@ruiyang2021 ruiyang2021 changed the title Adding a combined version define in MaterialX C++ Add a combined version define in MaterialX C++ Sep 27, 2024
@jstone-lucasfilm
Copy link
Member

This looks very promising, @ruiyang2021, and I think the unit test makes a good addition (though we may want to refine exactly where it's added).

To resolve the EasyCLA error message above, feel free to follow the two links they provide in the message, and let us know how things go!

@ruiyang2021
Copy link
Contributor Author

Gotcha, thank you! Going to sort the CLA with my manager. By the way, I'll be on holiday for 3 weeks starting from next Monday. Can come back to this one in 3 weeks if I have to leave something unfinished today

@ruiyang2021
Copy link
Contributor Author

Hi @jstone-lucasfilm, I just passed the CLA check :)
Would it be better to add the test in Document.cpp instead? Maybe under "// Test version integers." or at the end of the file?

@jstone-lucasfilm
Copy link
Member

Excellent, thanks @ruiyang2021!

For the location of the new test, I would actually recommend CoreUtil.cpp, which has tests for other MaterialXCore functions at global scope:

https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXTest/MaterialXCore/CoreUtil.cpp

ruiyang2021 and others added 4 commits October 23, 2024 17:40
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
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.

Great work, thanks @ruiyang2021!

@jstone-lucasfilm jstone-lucasfilm merged commit 1734e6d into AcademySoftwareFoundation:main Oct 24, 2024
34 checks passed
@ruiyang2021
Copy link
Contributor Author

Appreciate the help!

@ruiyang2021 ruiyang2021 deleted the add_combined_version branch October 24, 2024 05:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants