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

GafferOSL : ShadingEngine - cast doubles to float or int if required #2547

Merged
merged 1 commit into from May 1, 2018

Conversation

donboie
Copy link
Contributor

@donboie donboie commented Apr 11, 2018

No description provided.

@@ -294,6 +294,22 @@ class RenderState
t.arraylen = 2;
return ShadingSystem::convert_value( value, type, src, t );
}
else if ( it->dataView.type.basetype == TypeDesc::DOUBLE )
Copy link
Member

Choose a reason for hiding this comment

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

This would accept a V3d as well as just a double, right? Is that intended? I don't think we support a V3f->float conversion because it's ambiguous, and I would expect the same to apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked we're dealing with a scalar double in ba245a1

else if ( it->dataView.type.basetype == TypeDesc::DOUBLE )
{
double doubleValue;
ShadingSystem::convert_value( &doubleValue, TypeDesc::DOUBLE, src, TypeDesc::DOUBLE );
Copy link
Member

Choose a reason for hiding this comment

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

This is fairly performance-critical code, so I think it's probably worthwhile preferring static_cast<const double *>( src ) over convert_value() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

favoured casts over function call in ba245a1

@andrewkaufman andrewkaufman added the pr-revision PRs that should not be merged because a Reviewer has requested changes. label Apr 30, 2018
@donboie
Copy link
Contributor Author

donboie commented Apr 30, 2018

Perhaps we need to rewrite the Cortex -> OSL type conversion to provide a wider range of conversions? I've largely been responsible for adding special cases and perhaps we have enough examples where we can replace all these ad-hoc conversions with something more systematic.

@andrewkaufman andrewkaufman removed the pr-revision PRs that should not be merged because a Reviewer has requested changes. label May 1, 2018
@johnhaddon johnhaddon merged commit 297705a into GafferHQ:master May 1, 2018
@johnhaddon
Copy link
Member

Thanks!

Perhaps we need to rewrite the Cortex -> OSL type conversion to provide a wider range of conversions?

I think the ideal might be for ShadingSystem::convert_value to just do what we want in the first place. I seem to recall seeing some changes to it since the version we're currently using in GafferHQ/dependencies. Might be worth exploring that, and then seeing if they'd be receptive to a PR for anything still missing?

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

3 participants