OpenShadingLanguage icon indicating copy to clipboard operation
OpenShadingLanguage copied to clipboard

RendererServices::good() no longer used when generating textures

Open DeclanRussell opened this issue 5 years ago • 5 comments

Problem

While doing some texture work I noticed that the RendererServices::good() function is no longer being called when generating textures. The function serves as an interface to validate texture handles retrieved from RendererServices::get_texture_handle(). This was removed as of a5d8381afbe1ad3c16cd3db7f6e1ddf5c50e0b1f and the texture handle is now just checked if it is non NULL. What are the plans for this API moving forward? If it is no longer going to be used there should probably be some indication that it is deprecated.

Versions

  • OSL branch/version: master - 1.11.0
  • OS: All
  • C++ compiler: N/A
  • LLVM version: N/A
  • OIIO version: N/A

DeclanRussell avatar Mar 02 '20 15:03 DeclanRussell

You are right. I am not sure what the original intent of the call was, but I don't think this API is really needed.

I can't think of a case where we need to validate the texture handle on the OSL side before trying to use it. The lookups themselves are supposed to signal the error when attempting to use a broken handle. Calling good() might tell you something is wrong, but it can't tell you what, so its not really useful for error reporting.

I think we should just deprecate/remove it.

fpsunflower avatar Mar 02 '20 17:03 fpsunflower

FWIW, demand-loaded GPU textures might want something like that. For when the texture file hasn't been opened, so size etc are unknown. An Optix 7 demand-loading library is coming soon.

ee33 avatar Mar 02 '20 21:03 ee33

In this case, I would still expect get_texture_handle to do the work of preparing the GPU representation. If the texture is invalid, you still need to generate some representation that allows the error to be reported later on.

I don't see a need for OSL itself to need to know if the handle is "good" or not. Only the renderer (which implements these methods) should care, so the extra virtual entry point seems redundant.

fpsunflower avatar Mar 02 '20 22:03 fpsunflower

It was there for a reason. I need to look at the older code to remember why. Was it needed during optimization?

lgritz avatar Mar 02 '20 22:03 lgritz

The spots where it was called before didn't really make much sense. It was only resetting the handle to null when it was not good(), even though a get_texture_handle call must, by definition always give you back the same thing.

If get_texture_handle() gives you back non-null, we assume its ok to use that at runtime, even if it happens to be a broken handle. It is up to the specifics of the gettextureinfo or texture calls to report the error to the user.

fpsunflower avatar Mar 02 '20 22:03 fpsunflower