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

Improvements to support locale, units and additional tests. #892

Conversation

ashwinbhat
Copy link
Contributor

@ashwinbhat ashwinbhat commented Mar 29, 2022

@ashwinbhat ashwinbhat marked this pull request as draft March 29, 2022 04:59
@ashwinbhat ashwinbhat marked this pull request as ready for review March 29, 2022 15:51
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 promising, and I've written up a few initial thoughts below.

resources/Materials/TestSuite/locale/numericformat.mtlx Outdated Show resolved Hide resolved
source/MaterialXCore/Node.cpp Outdated Show resolved Hide resolved
source/MaterialXFormat/XmlExport.cpp Outdated Show resolved Hide resolved
source/MaterialXCore/Document.cpp Outdated Show resolved Hide resolved
source/MaterialXCore/Unit.cpp Show resolved Hide resolved
source/MaterialXCore/Util.cpp Outdated Show resolved Hide resolved
source/MaterialXCore/Util.h Outdated Show resolved Hide resolved
source/MaterialXFormat/Util.cpp Outdated Show resolved Hide resolved
source/MaterialXRender/Util.cpp Outdated Show resolved Hide resolved
source/MaterialXTest/MaterialXCore/Unit.cpp Outdated Show resolved Hide resolved
@ashwinbhat ashwinbhat force-pushed the adsk_contrib/locale_etc_to_main branch from 1fae076 to 665459b Compare April 6, 2022 00:54
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 changelist is definitely starting to come together, though I had some questions below about Document::addNodeDefFromGraph and its role in MaterialXCore.

source/MaterialXCore/Document.cpp Outdated Show resolved Hide resolved
source/MaterialXCore/Util.cpp Outdated Show resolved Hide resolved
ashwinbhat and others added 3 commits April 18, 2022 14:00
- Update Document::addNodeDefFromGraph to support Namespaces
- Add utilities
   - UnitConverterRegistry::convertToUnit
   - isValidNamespace
   - joinstring
- Use C Locale for string stream operations.
- Add tests for flatten, Namespaces, unit and locale content.
Minor updates to tests and remove unused headers.
@ashwinbhat ashwinbhat force-pushed the adsk_contrib/locale_etc_to_main branch from 1d6bc3a to 6ea8d33 Compare April 18, 2022 21:42
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 good to me, thanks @ashwinbhat!

@ashwinbhat
Copy link
Contributor Author

This looks good to me, thanks @ashwinbhat!

Thanks @jstone-lucasfilm for fixing it up. Appreciate it!

@jstone-lucasfilm jstone-lucasfilm merged commit 2dfd757 into AcademySoftwareFoundation:main Apr 19, 2022
@ashwinbhat ashwinbhat deleted the adsk_contrib/locale_etc_to_main branch April 28, 2022 23:57
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
- Add utilities UnitConverterRegistry::convertToUnit and MaterialX::joinStrings.
- Use C Locale for string stream operations (autodesk-forks#1183).
- Add tests for unit and locale content.
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