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 round node to data libraries #1678

Merged

Conversation

ld-kerley
Copy link
Contributor

Mirror the existing ceil node definition.

This is now being submitted for inclusion in main - previous PR (#1661) closed

@ld-kerley ld-kerley mentioned this pull request Jan 27, 2024
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, @ld-kerley, and I'm CC'ing @crydalch who brought up the need for this node in #1365.

@jstone-lucasfilm jstone-lucasfilm changed the title add 'round' node Add round node to data libraries Jan 28, 2024
@crydalch
Copy link
Contributor

Thanks @jstone-lucasfilm, and thank you @ld-kerley for the PR this looks great!

There was some discussion previously, when the issue was first raised, about the questions of the round() function, depending on the renderer/shading language. Jonathan pointed out that GLSL has some ambiguity, specifically when the input is 0.5: https://registry.khronos.org/OpenGL-Refpages/gl4/html/round.xhtml

Does round always round up/down consistently? The other issues raise previously were largely centered around float-to-int conversion, though there may have been other questions around behavior across hardware/software; because of those issues we just added integer signatures to floor and ceil (#1362).

Apart from the consistency question, this looks awesome. Thank you for adding this node!

@jstone-lucasfilm
Copy link
Member

Thanks for putting this together, @ld-kerley, and in order to resolve the ambiguity of the 0.5 input case, one option would be to specify that we expect roundEven behavior as documented here:

https://registry.khronos.org/OpenGL-Refpages/gl4/html/roundEven.xhtml

This would then become the expected behavior for the MaterialX round node in all shading languages.

@ld-kerley
Copy link
Contributor Author

@jstone-lucasfilm are you suggesting that we would impose the roundEven requirement on all of the other shader languages as well? I'm not sure how I feel about that, and also that perhaps reading "round" as the node name is confusing with respect to it being a subtly different implementation that I'd expect (speaking as a long time user of OSL - an non-GLSL shader writer).

@jstone-lucasfilm
Copy link
Member

@ld-kerley I wouldn't propose that we change the meaning of the round function in any shading language (certainly not in OSL!), but we may want to define the round node in MaterialX so that it has no ambiguity in its behavior.

In GLSL, for example, the round function has implementation-specific behavior when the input has a fractional value of exactly 0.5, and it would be good to define our MaterialX node so that this ambiguity is not present.

The roundEven function in GLSL looks like a proposal from Khronos to resolve this ambiguity, so it seems worthwhile to consider this as the behavior we would select in MaterialX, but I'm completely open to any alternative interpretation that has no ambiguity in its behavior.

Do you know if OSL has a corresponding resolution to the issue of ambiguous behavior when the fractional component is exactly 0.5?

@jstone-lucasfilm
Copy link
Member

To bring some of the OSL specification language into the conversation, here is how it specifies its round function:

round returns the closest integer to x, in either direction;

Do you happen to know how this function handles the case when the fractional component is exactly 0.5, so that neither the upper or lower integer is technically closer?

@jstone-lucasfilm
Copy link
Member

Bringing another reference point into the discussion, here is how MSL (Metal Shading Language) defines their round node:

Return the integral value nearest to x, rounding halfway cases away from zero.

https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf

This matches the definition of round in the C++ language as well, so it might be a really good choice for MaterialX:

Returns the integral value that is nearest to x, with halfway cases rounded away from zero.

https://cplusplus.com/reference/cmath/round/

@lgritz
Copy link
Contributor

lgritz commented Feb 6, 2024

OSL implements it as std::roundf, which says

 The round() functions return the integral value nearest to x rounding half-way
 cases away from zero, regardless of the current rounding direction.

@lgritz
Copy link
Contributor

lgritz commented Feb 6, 2024

So it looks like OSL and MSL both were constructed to match C/C++ round.

@lgritz
Copy link
Contributor

lgritz commented Feb 6, 2024

I will clarify the OSL docs to reflect what it does in practice.

In general, the OSL philosophy was that whenever there is a standard library function that appears to be analogous to an identically named C stdlib function, its behavior under "safe" conditions should match C unless there is a very compelling reason to deviate.

(An example of safe/unsafe deviation from C is that acos(x > 1.0) will return NaN and raise a floating point exception in C, but in OSL it will behave as if its input was clamped to [-1,1] so as to not introduce any NaNs into the shader.)

@jstone-lucasfilm
Copy link
Member

Thanks for confirming, @lgritz, and this sounds like the right choice for MaterialX as well.

Although the GLSL implementation of round doesn't currently guarantee this behavior, I think it's fine to use GLSL round in this initial implementation of the node, as long as we state the exact behavior that we expect in the specification.

Later on, we can refine the GLSL implementation to exactly match the behavior of OSL and MSL, and can additionally check with NVIDIA to make sure MDL aligns with this behavior as well.

@lgritz
Copy link
Contributor

lgritz commented Feb 6, 2024

I might have some insight about why OpenGL likes roundEven. Looking in OIIO's simd.h header, I see the following interesting comment that I wrote many years ago:

/// Per-element round to nearest integer.
/// CAVEAT: the rounding when mid-way between integers may differ depending
/// on hardware. Intel SSE/AVX does "banker's rounding" (to nearest even
/// integer) but std::round() says to round away from 0 regardless of
/// current rounding mode (but that is multiple instructions on x64).
/// USE WITH CAUTION, and maybe avoid this if it is critical to exactly
/// match std::round().
vfloat4 round (const vfloat4& a);

So this implies that it may be common for hardware to implement "even" rounding directly as a single instruction, but matching C is lightly more expensive by requiring multiple instructions (at least on x86, maybe on certain GPUs too?). So if you don't particularly care about the specifics of how the rounding works at i+0.5 values, "even" may be slightly more efficient.

@ld-kerley
Copy link
Contributor Author

I just rebased on top of the recent MDL versioning changes - it would be great if someone could just validated I updated in the way that's intended. I don't have a mechanism to validate MDL changes currently. Perhaps @krohmerNV ?

@krohmerNV
Copy link
Contributor

you missed the package names (I made it more explicit with the changes you mentioned)

export core::color4 mx_round_color4(
	core::color4 mxp_in = core::mk_color4(0.0, 0.0, 0.0, 0.0)
)
	[[
		anno::description("Node Group: math")
	]]
{
	return core::mk_color4(::math::round(core::mk_float4(mxp_in)));
}

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.

Looks good to me, thanks @ld-kerley!

@jstone-lucasfilm jstone-lucasfilm merged commit 4eadb47 into AcademySoftwareFoundation:main Feb 9, 2024
31 checks passed
@ld-kerley ld-kerley deleted the adding_round_node branch May 21, 2024 21:44
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.

5 participants