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

Update overlay node logic to match reference #1539

Conversation

rasmusbonnedal
Copy link
Contributor

@rasmusbonnedal rasmusbonnedal commented Sep 22, 2023

This commit changes the overlay node to match the overlay operation in the Adobe PDF spec.

Overlay(cb, cs) = { 2 * cs * cb                 [if cb <= 0.5]
                    1 - 2 * (1 - cs) * (1 - cb) [if cb > 0.5]

(cb = B, cs = F in MaterialX specification notation)

It also changes the implementation to graph based instead of code snippets.

This commit changes the overlay node to match the overlay operation
in the Adobe PDF spec.

Overlay(cb, cs) = { 2 * cs * cb                 [if cb <= 0.5]
                    1 - 2 * (1 - cs) * (1 - cb) [if cb > 0.5]

(cb = B, cs = F in MaterialX specification notation)

It also changes the implementation to graph based instead of
code snippets.
This is done to make the test cover more interesting input
@rasmusbonnedal
Copy link
Contributor Author

Before the fix:
image

@rasmusbonnedal
Copy link
Contributor Author

After the fix:
image

@rasmusbonnedal
Copy link
Contributor Author

before.pdf
after.pdf

Copy link
Contributor

@kwokcb kwokcb left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for spending the time to do this.
I left a small suggestion to add docs for the node since it's being edited but this is a minor thing.

libraries/stdlib/stdlib_defs.mtlx Show resolved Hide resolved
@jstone-lucasfilm
Copy link
Member

Great work, @rasmusbonnedal, and I really like the decision to implement this node as a language-independent graph. I don't see any issues that concern me in this change, though I'd like to run this through local tests before merging.

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 @rasmusbonnedal!

@jstone-lucasfilm jstone-lucasfilm changed the title Fix overlay node Update overlay logic to match reference Oct 19, 2023
@jstone-lucasfilm jstone-lucasfilm changed the title Update overlay logic to match reference Update overlay node logic to match reference Oct 19, 2023
@jstone-lucasfilm jstone-lucasfilm merged commit 78d2762 into AcademySoftwareFoundation:main Oct 19, 2023
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.

None yet

4 participants