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

Support to export texture sets as single output per texture map #25

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from

Conversation

moonyuet
Copy link
Member

@moonyuet moonyuet commented Dec 5, 2024

Changelog Description

This PR is to add support to export texture sets as single output per texture map.
Resolve #20

Additional review information

We need to discuss what the best way to merge the maps with alpha

Testing notes:

  1. Launch SP
  2. Enable Export Texture Sets as Texture Output
    image
  3. Publish

@moonyuet moonyuet requested review from BigRoy and LiborBatek December 5, 2024 10:31
@moonyuet moonyuet self-assigned this Dec 5, 2024
@moonyuet moonyuet added the type: enhancement Improvement of existing functionality or minor addition label Dec 5, 2024
@moonyuet
Copy link
Member Author

moonyuet commented Dec 5, 2024

I made this draft as it hits stones during the testing in my side.
@BigRoy I tested the code from my side and find out that oiio tools are not working for both tx extractor and also the new extractor for this PR. It could be something wrong from my side.
@LiborBatek can you test it from your side too?

Comment on lines 199 to 201
# Function to remove textureSet from filepath
def remove_texture_set_token(filepath, texture_set):
return filepath.replace(texture_set, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? Do we care about the texture set name at all?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need to have new filename for the single output :).

# Store the instance in the original instance as a member
instance.append(image_instance)

def create_image_instance_by_map_filtering(self, instance, outputs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need a completely isolated function? Seems like a LOT of duplicated code? Why can't de-duplicate and only change what is actually different?

What makes this so massively different?

)


def convert_texture_maps_for_udim_export(staging_dir, image_outputs, has_udim=False):
Copy link
Contributor

Choose a reason for hiding this comment

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

This function name is confusing - it doesn't 'convert' anything. It just gets the sequence filepaths. Please add a docstring, return type and improve function name,

Comment on lines 39 to 42
oiio_cmd = oiio_tool_args + source_maps + [
"--over", "-o",
dest_map
]
Copy link
Contributor

Choose a reason for hiding this comment

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

If I read the oiiotool documentation right the --over flag will only ever overlay the TWO top images, not ALL input images? As such this may only overlay TWO images at most even if source maps may be 5 texture sets?

dest_map
]

subprocess_args = " ".join(oiio_cmd)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be joining this ourselves and pass the joined command to run_subprocess but instead pass it the list of arguments.

Comment on lines 44 to 53
subprocess_args = " ".join(oiio_cmd)

env = os.environ.copy()
env.pop("OCIO", None)
log.info(" ".join(subprocess_args))
try:
run_subprocess(subprocess_args, env=env)
except Exception:
log.error("Texture maketx conversion failed", exc_info=True)
raise
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be joining this ourselves and pass the joined command to run_subprocess but instead pass it the list of arguments.

Suggested change
subprocess_args = " ".join(oiio_cmd)
env = os.environ.copy()
env.pop("OCIO", None)
log.info(" ".join(subprocess_args))
try:
run_subprocess(subprocess_args, env=env)
except Exception:
log.error("Texture maketx conversion failed", exc_info=True)
raise
env = os.environ.copy()
env.pop("OCIO", None)
log.info(" ".join(oiio_cmd))
try:
run_subprocess(oiio_cmd, env=env)
except Exception as exc:
raise RuntimError("Flattening texture stack to single output image failed") from exc

Also, please add a todo as to WHY we need to remove OCIO var? What does it break if it remains specified?

Comment on lines 72 to 76
if "exportTextureSetsAsOneOutput" not in instance.data["creator_attributes"]:
self.log.debug(
"Skipping to export texture sets as single texture output.."
)
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if "exportTextureSetsAsOneOutput" not in instance.data["creator_attributes"]:
self.log.debug(
"Skipping to export texture sets as single texture output.."
)
return
if not instance.data.get("creator_attributes", {}).get(
"exportTextureSetsAsOneOutput", False):
self.log.debug(
"Skipping to export texture sets as single texture output.."
)
return

Comment on lines 84 to 90
for representation in list(representations):
dest_files = representation["files"]
is_sequence = isinstance(dest_files, (list, tuple))
if not is_sequence:
dest_image_outputs = [dest_image_outputs]
if "udim" in representation:
has_udim = True
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is somehow assuming that there is only ever one representation? Or? This feels like it's prone to confusion since we're getting values from all representations, but end up processing only specific images from what happens to be the 'last representation' in the iterations?

@BigRoy
Copy link
Contributor

BigRoy commented Dec 5, 2024

I tested the code from my side and find out that oiio tools are not working for both tx extractor and also the new extractor for this PR. It could be something wrong from my side.

Please report the errors you get :)

@moonyuet
Copy link
Member Author

moonyuet commented Dec 5, 2024

I tested the code from my side and find out that oiio tools are not working for both tx extractor and also the new extractor for this PR. It could be something wrong from my side.

Please report the errors you get :)

No errors, and nothing runs in my side, but the integrator does not transfer anything.

@BigRoy
Copy link
Contributor

BigRoy commented Dec 5, 2024

No errors, and nothing runs in my side, but the integrator does not transfer anything.

Please share a publish JSON report - note that it skips the maketx if OCIO colorspace management is disabled but it should log a warning then.

@LiborBatek
Copy link
Member

Ok so I have tested the workflow with multiple materials and so far no failure but as @moonyuet already mentioned no integration happens for images. Also not sure if correct to have exactly the same image outputs as they got multiplied when having more than single material (probably should end up as UDIM tiles instead) as it will probably fail during integration part of publishing...

something like:

mat01_diffuse > main_diffuse.1001
mat02_diffuse > main_diffuse.1002
etc.

here you can see the resulting publish instances ...and @BigRoy also enclosing the JSON report for inspection of not integration reasoning.

Screenshot 2024-12-16 164436

SPainter-publish-report-241216-16-48.json.txt

Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Publishing finnished without any error but only workfile been integrated...

no image products been integrated on AYON,

again enclosing JSON report for inspection
SP-publish-report-250106-17-53.json.txt

…ltiple-texture-sets-to-export-as-one-texture-output
@moonyuet moonyuet requested a review from LiborBatek February 13, 2025 08:55
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Again publishing produces just workfile product...nothing else

Screenshot 2025-02-13 103933

and enclosing full JSON report:

SP-publish-report-250213-10-39.json.txt

@moonyuet moonyuet requested a review from LiborBatek February 18, 2025 09:38
@moonyuet moonyuet marked this pull request as ready for review February 18, 2025 09:38
Copy link
Member

@LiborBatek LiborBatek left a comment

Choose a reason for hiding this comment

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

Finally it produces also the image products but it produces all texture set separately which is not desired I guess (as it should act as single texture set??)

here is my model with 2 texture sets
image

and published files:

image

So I guess it behaves as normally without this PR??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Improvement of existing functionality or minor addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AY-7195_Allow project with multiple texture sets to export as one texture output
3 participants