Maya looks: support for native Redshift texture format

[ghost]

Brief description

Add support for native Redshift textures handling. Closes #2599

Description

Uses Redshift's Texture Processor executable to convert textures being used in renders to the Redshift ".rstexbin" format.

Additional info

Essentially emulating what maketx does by passing a number of arguments to a subprocess and returning a status code to indicate whether the operation was successful or not.

[ynbot]

Task linked: OP-2524 Maya looks: support for native Redshift texture format

[iLLiCiTiT]

He's right

[iLLiCiTiT]

Note: I think we should validate this environment during validation, only when is needed if it's possible to know.

[ghost]

Ah, I see! Thank you, I'll take a look about moving that

[iLLiCiTiT]

That's something I don't know. Maybe it's not needed or impossible to validate before this plugin, can't tell.

[ghost]

I see, thank you!

[antirotor]

This is more complex as this is set only if studio is using and has configured Redshift. So I think the logic should be - add option make redshift texture on the same place as we have with make tx (look instance) and then probably have validator that will run on look family and will validate this env var.

[iLLiCiTiT]

Few notes that are connected to each other:

• I would recommend to implement function that will return path to redshift tool (single function job, easy to check what it does)
• also would recommend to use run_subprocess from openpype.lib which would do what you do here with subprocess
• when those 2 above would be implemented this function would have 2 lines and is not needed at all...
• if you know what the arguments will be, it is always better to pass launch arguments as list instead of string to subprocess function
  • more safe for multiplatform support
  • escape_space will not be needed in that case
  • but it has few disadvantages

  # This will work (arg is just for example)
  args = [texture_processor_path, source_path, "-arg", "value"]
  # This will not work
  args = [texture_processor_path, source_path, "-arg value"]

At this stage are these notes just recommendation because I don't know how it will be used.

[ghost]

Thank you! This is helpful, makes me think whether we should consider refactoring the maketx function as well to simplify a little bit.

Will hop on to something else in the meantime. I appreciate your time!

[BigRoy]

Not a huge issue - but this does feel off here since unlike the rest of the methods here this Redshift binary isn't actually vendorized? I do like that there's unison in the function itself however - nonetheless it's not perfect.

[ghost]

Thank you @BigRoy, you bring up a good point. I wonder where it would be good to place the function then @antirotor @iLLiCiTiT would love your take on where I should be placing this

[antirotor]

I would probably define separate ENV var for the tool that should be defined in the tool itself to use it.

[m-u-r-p-h-y]

I would probably define separate ENV var for the tool that should be defined in the tool itself to use it.

is it necessary? why can't we use the mandatory {REDSHIFT_COREDATAPATH}/bin/redshiftTextureProcessor.exe (and the appropriate lnx and mac variant)

[ghost]

Yes thank you I know :D

[m-u-r-p-h-y]

look with textures extracted and published successfully

[BigRoy]

look with textures extracted and published successfully

@m-u-r-p-h-y - Shouldn't that produce .rstexbin files with this PR instead of .tx files?

Did you enable to rstex attribute on the look instance?

[BigRoy]

Isn't it odd that we define something like a TextureProcessor base class but then end up needing to pass different types of input variables for the conversion - shouldn't those at least have the same method signatures so that when adding an extra type of processor it's just a means of implementing that base class?

I'd have expected this logic to just be:

for processor in texture_processors:
    processor.process(src_filepath,
                                dest_filepath,
                                source_hash=texture_hash,
                                ocio_config=color_config,
                                color_conversion=color_convert)

No?

Also,

self.log.info("redshift texture converter has returned nothing")

That doesn't seem like "info" but sounds more like a warning or error even.

[m-u-r-p-h-y]

look with textures extracted and published successfully

@m-u-r-p-h-y - Shouldn't that produce .rstexbin files with this PR instead of .tx files?

Did you enable to rstex attribute on the look instance?

Ohh you are completely right. I had old look instance in my scene.

Redid the test and it is correctly producing rstexbin files.

my question is, do we really need to specify the format in the instance? Isn't it clear that we should do

arnold -> tx
redshift -> rxtexbin

this way it opens a space for human error as I already demonstrated . . .

[BigRoy]

my question is, do we really need to specify the format in the instance? Isn't it clear that we should do
arnold -> tx
redshift -> rxtexbin

Actually I'd say yes. I believe Redshift can actually render both .tx and .rstexbin files but they serve a different purpose. (I might be wrong however!) I recall having done a test on our infrastructure and it was usually faster to render without .rstexbin files and use regular textures or .tx files. But then again, I might be misremembering.

Check the Render Time Considerations

this way it opens a space for human error as I already demonstrated . . .

🙉 🙈 🙊

[antirotor]

I agree that we need to specify format, but we'll need to add in the future validators to deal with unsupported features enabled on existing render instance - for example arnold/vray/renderman won't understand .rstexbin, renderman will not understand .tx/.rstexbin but will read .tex and so on.

[m-u-r-p-h-y]

I agree that we need to specify format, but we'll need to add in the future validators to deal with unsupported features enabled on existing render instance - for example arnold/vray/renderman won't understand .rstexbin, renderman will not understand .tx/.rstexbin but will read .tex and so on.

the toggle in render instance could only state

• convert textures to native format

and we should know, depending on renderer, which format to use . . .

[ghost]

Would that be ideal? To have that toggle on the instance?

[m-u-r-p-h-y]

Would that be ideal? To have that toggle on the instance?

we have this currently

and it is not ideal definitely

[ghost]

Okay hound I'll fix it buddy, and get you a treat, too

[m-u-r-p-h-y]

tested successfully

Maya2023 and RS 5.3.09

[iLLiCiTiT]

I have to say few notes regarding subprocess arguments. If it is possible (and here it is possible) arguments should be passed to subprocess functions as list of strings so you don't have to call anything like escape_space.

Also I agree with @BigRoy about those explicit arguments for TextureProcessor. If one has different arguments then other then what is the reason of having them in abstract class... They should not be prepared for specific processor before they're passed to their processing method and all of the methods should expect the same arguments.

• colorconvert variable should be split to src_colorspace and dst_colorspace which are passed to process method
• the same for color_config which is just path to config file where colorspaces can be found
• the same for texture_hash
• the same for staging_dir (handling converted varible probably should not happen if MakeTX is not executed?)
  In that case eash processor's process method arguments should expect these arguments:

def process(
    self,
    src_filepath,
    staging_dir,
    texture_hash,
    color_config_path,
    src_colorspace,
    dst_colorspace
):
    pass

So process method of MakeTX can be changed to:

    def process(
        self,
        src_filepath,
        staging_dir,
        texture_hash,
        color_config_path,
        src_colorspace,
        dst_colorspace
    ):
        """Make `.tx` using `maketx` with some default settings.
        The settings are based on default as used in Arnold's
        txManager in the scene.
        This function requires the `maketx` executable to be
        on the `PATH`.
        Args:
            ...fill
        Returns:
            str: Output filepath/s.
        """
        from openpype.lib import get_oiio_tools_path

        maketx_path = get_oiio_tools_path("maketx")

        if not os.path.exists(maketx_path):
            print(
                "OIIO tool not found in {}".format(maketx_path))
            raise AssertionError("OIIO tool not found")

        dirpath, filename = os.path.split(src_filepath)
        fname, ext = os.path.splitext(filename)
        dst_filepath = os.path.join(staging_dir, "resources", fname + ".tx")
        if not os.path.exists(os.path.dirname(dst_filepath)):
            os.makedirs(os.path.dirname(dst_filepath))

        subprocess_args = [
            maketx_path,
            "-v",  # verbose
            "-u",  # update mode
            # unpremultiply before conversion (recommended when alpha present)
            "--unpremult",
            "--checknan",
            # use oiio-optimized settings for tile-size, planarconfig, metadata
            "--oiio",
            "--filter", "lanczos3",
            src_filepath,
        ]

        additional_args = []
        if src_colorspace and dst_colorspace and color_config_path:
            additional_args.extend([
                "--colorconfig", color_config_path,
                "--colorconvert", src_colorspace, dst_colorspace
            ])
        
        if texture_hash:
            additional_args.extend([
                "--sattrib",
                "sourceHash",
                texture_hash
            ])

        subprocess_args.extend(additional_args)
        subprocess_args.extend(["-o", dst_filepath])

        kwargs = {}
        if hasattr(subprocess, "CREATE_NO_WINDOW"):
            kwargs["creationflags"] = subprocess.CREATE_NO_WINDOW

        subprocess.call(
            subprocess_args,
            stderr=subprocess.STDOUT,
            **kwargs
        )

        return dst_filepath

[antirotor]

This relates on comment above that get_redshift_tool() should be static member of MakeRSTexbin class.

Also I would check for the tool presence, not only REDSHIFT_COREDATAPATH as that is already done in get_redshift_tool()

And last - lets use the new PublishValidationError available and compatible with with the old publisher.

Suggested change

	if "REDSHIFT_COREDATAPATH" not in os.environ:
	raise RuntimeError("Must have Redshift available.")
	texture_processor_path = get_redshift_tool("redshiftTextureProcessor")
	texture_processor_path = get_redshift_tool("redshiftTextureProcessor")
	if not texture_processor_path:
	raise PublishValidationError("Must have Redshift available.", title="Make RSTexBin texture")

This will ofc require from openpype.pipeline import PublishValidationError import.

[antirotor]

Again, this exception should re-raise PublishValidationError and pass some info there, prints will get lost.

[mkolar]

We should not be raising PublishValidationError in extractors. That is too late and should be standard error exception. validation error is reserved for things that the artist can affect and fix and should be caught in the validation step.

[antirotor]

I guess this is the same problem and should be handled with PublishValidationError and not prints.

[antirotor]

This could be simplified:

Suggested change

	for processor in processors:
	if processor is MakeTX:
	destinations[source] = self.resource_destination(
	instance, source, MakeTX
	)
	elif processor is MakeRSTexBin:
	destinations[source] = self.resource_destination(
	instance, source, MakeRSTexBin
	)
	for processor in processors:
	destinations[source] = self.resource_destination(
	instance, source, processor
	)

[antirotor]

What about moving those to the processor class, because I have a feeling that the extension is defined by the processor. Because you could then do:

Suggested change

	if processor == MakeTX:
	ext = ".tx"
	elif processor == MakeRSTexBin:
	ext = ".rstexbin"
	ext = processor.ext

Provided there's something like:

class MakeTX(TextureProcessor):
    ext = ".tx"
    ...