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

Added GetProcessorFromConfigs variants for displays and views. #1808

Merged
merged 3 commits into from
Aug 29, 2023

Conversation

num3ric
Copy link
Contributor

@num3ric num3ric commented Jun 26, 2023

Yield a single processor from a color space in config (a) to a display-view in config (b), through the interchange space.

Let me know if we think we need test coverage for these.

@num3ric
Copy link
Contributor Author

num3ric commented Jun 26, 2023

(Would be good for the library to have a .clang-format file.)

@remia
Copy link
Collaborator

remia commented Jun 29, 2023

Thanks @num3ric, sounds like a good idea to add some unit tests inspired from the existing tests on other GetProcessorFromConfigs(). I think we should create an issue for the clang-format if not already existing (didn't found one), it was discussed some time ago but got lost.

@remia
Copy link
Collaborator

remia commented Jul 13, 2023

There is a conflict on the branch following some recent merges in main.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
…argument last.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
@num3ric
Copy link
Contributor Author

num3ric commented Jul 27, 2023

Added unit tests. Also moved the direction argument last since in this case it reverses the entire chain of transforms.

Copy link
Collaborator

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

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

Thanks Eric! No need to make any changes (the two notes are just to myself, for later).

if (!p2)
{
throw Exception("Can't create the processor for the destination config "
"and the destination color space.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

"color space" --> "display/view"

const char * dstDisplay,
const char * dstView,
TransformDirection direction)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code you used as the model for this method was recently removed and replaced with GetInterchangeRolesForColorSpaceConversion. I think some refactoring will be needed later, but it's ok to reintroduce this here for now.

@michdolan michdolan merged commit de6d62d into AcademySoftwareFoundation:main Aug 29, 2023
22 checks passed
brkglvn01 pushed a commit to brkglvn01/OpenColorIO that referenced this pull request Oct 23, 2023
…mySoftwareFoundation#1808)

* Added GetProcessorFromConfigs variants for displays and views.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>

* Added display-view processor from two configs tests, moved direction argument last.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>

---------

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Brooke <beg9562@rit.edu>
doug-walker pushed a commit to autodesk-forks/OpenColorIO that referenced this pull request Dec 6, 2023
…mySoftwareFoundation#1808)

* Added GetProcessorFromConfigs variants for displays and views.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>

* Added display-view processor from two configs tests, moved direction argument last.

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>

---------

Signed-off-by: Eric Renaud-Houde <eric.renaud.houde@gmail.com>
Co-authored-by: Michael Dolan <michdolan@gmail.com>
Signed-off-by: Doug Walker <Doug.Walker@autodesk.com>
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.

None yet

4 participants