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 thumbnail generation script #64

Merged
merged 6 commits into from
Nov 28, 2023

Conversation

beersandrew
Copy link
Contributor

- added updates to height / width
- fixed typo in windows example call
@Lucas3Dspain
Copy link

I think there is a bug with UsdGeom.BBoxCache().
I would expect that using UsdGeom.Tokens.default_ will use all geo for both render and proxy purposes.

I'm trying to generate a thumbnail for an ALAB asset but the bbox is not being calculated properly.

If I do something like UsdGeom.BBoxCache(Usd.TimeCode.Default(), [UsdGeom.Tokens.proxy, UsdGeom.Tokens.render, UsdGeom.Tokens.default_]) it works for me.

Also, is there a way to force usdrender to render the render purpose or provide a rendersettigns prim?

@marktucker
Copy link

I think this BBoxCache behavior is not a bug. Specifying default does not imply render and/or proxy. This matches hydra's behavior where if you don't explicitly say you want proxy and/or render, you won't see proxy and/or render purpose geometry (but in hydra, default is always imlpied, which may be different that BBoxCache).

usdrecord accepts a --purposes command line option. I don't think a rendersettings command line option is required because that choice can be specified on the stage's root layer as root prim metadata.

@marktucker
Copy link

Oh, and also it's generally a bad idea to use Usd.TimeCode.Default() when querying data from a stage (unless you're really sure you only care about default values). If an asset has time sampled data, you really want to pick a specific time (even if it's just 1.0 or 0.0). Requesting data from the Default time will ignore time samples, but requesting data for time 0 will return a time samples preferentially, but fall back to the default value if there are no time samples. Much more likely to give good results on time sampled assets.

- moved hardcoded names to global constants
- added --apply-thumbnail argument to control if the thumbnail is written back to the asset, added to ReadMe
- replaced axis modification to modify the camera & lights rather than the asset
- set BBox calc to timeCode 0.0 by default instead of default
@marktucker
Copy link

Thanks @beersandrew ! Nothing further from me.

@Lucas3Dspain
Copy link

Lucas3Dspain commented Nov 4, 2023

Hi, I have been doing some testing. This is looking awesome!

Could we set the model:drawMode to default in the defaultPrim so we don't get a render of the cards?
Only in the render file, make sure we don't save this edit in the asset.

    # Set drawMode to default
    defaultPrim_path = subject_stage.GetDefaultPrim().GetPath()
    subject_defaultPrim = camera_stage.GetPrimAtPath(defaultPrim_path)
    geom_model_api = UsdGeom.ModelAPI(subject_defaultPrim)
    geom_model_api.CreateModelDrawModeAttr(UsdGeom.Tokens.default_)
    camera_stage.Save()

Some things I found that could be improved:

  • Add argument for purposes, by default ["default"]
  • Use purposes in the usdrecord command
  • Use purposes to compute the bbox.
  • Make a function to gather the asset dependencies (sublayers, references, textures ...) instead of file_list = [usd_file, image_name]
# some code I had laying around, there might be better ways to do this.
def list_resolved_dependencies(stage_path):
    resolved_dependencies = []
    layers, assets, unresolved = UsdUtils.ComputeAllDependencies(stage_path)
    resolved_layers = [os.path.normpath(layer.realPath) for layer in layers]
    resolved_dependencies.extend(resolved_layers)
    resolved_assets = [os.path.normpath(asset) for asset in assets]
    resolved_dependencies.extend(resolved_assets)
    return resolved_dependencies
  • Fix .with_suffix() It errors. Looks like with_suffix() expects something starting with .
# Alternative without using .with_suffix()
def zip_results(usd_file, image_name, is_usdz):
    file_list = list_resolved_dependencies(usd_file)
    usdPath = Path(usd_file)

    if is_usdz:
        file_list.append(usd_file.replace(usdPath.suffix, THUMBNAIL_LAYER_SUFFIX))

    usdz_file = usd_file.replace(usdPath.suffix, THUMBNAIL_LAYER_SUFFIX)
    cmd = ["usdzip", "-r", usdz_file] + file_list
    run_os_specific_usdrecord(cmd)
  • Use run_os_specific_usdrecord() instead of subprocess.run() in zip_results()
  • Do is_usdz = usd_file.endswith(".usdz") instead of is_usdz = ".usdz" in usd_file
  • <asset>_Thumbnail.usda gets generated even without --apply-thumbnail. Lets do something like this:
subject_stage = create_usdz_wrapper_stage(usd_file) if is_usdz and args.apply_thumbnail else Usd.Stage.Open(usd_file)
  • What is Path(THUMBNAIL_FOLDER_NAME).mkdir(parents=True, exist_ok=True) for?. If that is a temp directory, can we remove it at the end?

Sorry for the long text 😅

- Set the drawMode to default, in case the given asset is in cardsMode, don't take a thumbnail of cards
- in the case of zipping, zip all assets, rather than just the given usd file + thumbnail file
- code cleanup with globals, suffixes
- Don't create a thumbnail.usda file in the case of applying the thumbnail directly
- delete thumbnail.usda in the case of zipping everything together given a usdz file
@beersandrew
Copy link
Contributor Author

Could we set the model:drawMode to default in the defaultPrim so we don't get a render of the cards? Only in the render file, make sure we don't save this edit in the asset.

    # Set drawMode to default
    defaultPrim_path = subject_stage.GetDefaultPrim().GetPath()
    subject_defaultPrim = camera_stage.GetPrimAtPath(defaultPrim_path)
    geom_model_api = UsdGeom.ModelAPI(subject_defaultPrim)
    geom_model_api.CreateModelDrawModeAttr(UsdGeom.Tokens.default_)
    camera_stage.Save()

Makes sense, I've made a commit to add this.

Some things I found that could be improved:

  • Add argument for purposes, by default ["default"]
  • Use purposes in the usdrecord command
  • Use purposes to compute the bbox.

I unfortunately have never dealt with purposes, I see the usdrecord function has the following complexity argument:

--complexity {low,medium,high,veryhigh}, -c {low,medium,high,veryhigh}

This doesn't seem to be the same thing as purposes to me, do you have a sample argument (and maybe a sample file?) I could test with purposes with usdrecord?

I've held off on any purposes changes until I understand this more clearly.

  • Make a function to gather the asset dependencies (sublayers, references, textures ...) instead of file_list = [usd_file, image_name]
# some code I had laying around, there might be better ways to do this.
def list_resolved_dependencies(stage_path):
    resolved_dependencies = []
    layers, assets, unresolved = UsdUtils.ComputeAllDependencies(stage_path)
    resolved_layers = [os.path.normpath(layer.realPath) for layer in layers]
    resolved_dependencies.extend(resolved_layers)
    resolved_assets = [os.path.normpath(asset) for asset in assets]
    resolved_dependencies.extend(resolved_assets)
    return resolved_dependencies

Thanks, I've added this.

  • Fix .with_suffix() It errors. Looks like with_suffix() expects something starting with .
# Alternative without using .with_suffix()
def zip_results(usd_file, image_name, is_usdz):
    file_list = list_resolved_dependencies(usd_file)
    usdPath = Path(usd_file)

    if is_usdz:
        file_list.append(usd_file.replace(usdPath.suffix, THUMBNAIL_LAYER_SUFFIX))

    usdz_file = usd_file.replace(usdPath.suffix, THUMBNAIL_LAYER_SUFFIX)
    cmd = ["usdzip", "-r", usdz_file] + file_list
    run_os_specific_usdrecord(cmd)

Thanks, I've added this.

  • Use run_os_specific_usdrecord() instead of subprocess.run() in zip_results()
  • Do is_usdz = usd_file.endswith(".usdz") instead of is_usdz = ".usdz" in usd_file
  • <asset>_Thumbnail.usda gets generated even without --apply-thumbnail. Lets do something like this:
subject_stage = create_usdz_wrapper_stage(usd_file) if is_usdz and args.apply_thumbnail else Usd.Stage.Open(usd_file)

Thanks, I've added all of these.

  • What is Path(THUMBNAIL_FOLDER_NAME).mkdir(parents=True, exist_ok=True) for?. If that is a temp directory, can we remove it at the end?

This is to create the folder to hold the thumbnails if the folder does not exist already, if it already exists then just ignore it. I could add some code here to delete the thumbnail and the folder in the case of creating a usdz result file, however this would require checking if there are other thumbnails in the same folder before deleting the folder, which I can do, but I think the folder itself makes sense, and it doesn't make sense to always delete it because that's where the result file is saved. To me there doesn't seem to be any necessary action here, however if that delete logic is valuable to you I can try to add it, I just worry about someone running it in some case, has a ton of thumbnails in the directory and blows away the directory, I guess checking that it's empty would protect against this though...

Sorry for the long text 😅

Thank you for all the suggestions! I really appreciate it 🙏

@Lucas3Dspain
Copy link

Thanks for the changes!

About the purposes, we will need to add a new arg that gets a list of strings.
then we need to pass that to the bbox and usdrecord.

usdrecord
The usdrecord command takes --purposes you need to pass it a string with the purposes separated by ", "
args.purposes = "default, renders"

def take_snapshot(image_name):
    renderer = get_renderer()
    cmd = ['usdrecord', '--camera', 'MainCamera', '--imageWidth', str(args.width), '--renderer', renderer, DEFAULT_THUMBNAIL_FILENAME, image_name]
    if args.purposes:
        cmd.extend(['--purposes', args.purposes])
    run_os_specific_command(cmd)
    os.remove(DEFAULT_THUMBNAIL_FILENAME)
    return image_name

bbox
For the bbox we should provide UsdGeom.Tokens instead of strings so we will need a small function to return the tokens from the strings, and then pass the list of tokens to the bbox command.

DEFAULT_BBOX_PURPOSES = [UsdGeom.Tokens.default_]
def pseudo_converter(args.purposes):
    if args.purpose:
        tokenized_purposes = []
        for purpose in args.purpose:
            tokenized_purpose = convert(purpose)
            tokenized_purposes.append(tokenized_purpose)
    else:
        tokenized_purposes = DEFAULT_BBOX_PURPOSES
    return tokenized_purposes

bboxCache = UsdGeom.BBoxCache(Usd.TimeCode(0.0), pseudo_converter(args.purposes))

Well, that's a suggestion only, implement it in the way that you think is best. 👍

This is to create the folder to hold the thumbnails if the folder does not exist already, if it already exists then just ignore it. I could add some code here to delete the thumbnail and the folder in the case of creating a usdz result file, however this would require checking if there are other thumbnails in the same folder before deleting the folder, which I can do, but I think the folder itself makes sense, and it doesn't make sense to always delete it because that's where the result file is saved. To me there doesn't seem to be any necessary action here, however if that delete logic is valuable to you I can try to add it, I just worry about someone running it in some case, has a ton of thumbnails in the directory and blows away the directory, I guess checking that it's empty would protect against this though...

Gocha! it looks like the join path was not working properly with full paths. (at least for me)
we could make these changes also to support THUMBNAIL_FOLDER_NAME = None

def generate_thumbnail(usd_file, verbose, extension):
    if verbose: 
        print("Step 1: Setting up the camera...")
    
    subject_stage = Usd.Stage.Open(usd_file)

    setup_camera(subject_stage, usd_file)
    
    if verbose:
        print("Step 2: Taking the snapshot...") 
    
    image_path = create_image_filename(usd_file, extension)
    return take_snapshot(image_path)
def create_image_filename(input_path, extension):
    if not isinstance(input_path, Path):
        input_path = Path(input_path)
    if THUMBNAIL_FOLDER_NAME:
        thumbnail_folder_dir = Path().joinpath(input_path.parent, THUMBNAIL_FOLDER_NAME)
    else:
        thumbnail_folder_dir = input_path.parent
    thumbnail_folder_dir.mkdir(parents=True, exist_ok=True)
    return str(Path().joinpath(thumbnail_folder_dir, input_path.name).with_suffix("." + extension)).replace("\\", "/")

- added support to specify which render purposes to be included in the bbox calculation as well as the render
- added support for passing in a fully qualified usd path
- small refactoring
- updated docs
@beersandrew
Copy link
Contributor Author

I've updated the branch with these changes

usdrecord The usdrecord command takes --purposes you need to pass it a string with the purposes separated by ", " args.purposes = "default, renders"

is this in the official docs somewhere? are there other arguments that you know about that aren't listed in the docs?

Let me know if the code changes make sense the way I did them and let me know if there are any other improvements you can think of

@Lucas3Dspain
Copy link

I only knew about the --purposes argument because @marktucker mentioned it before. I tried it and it works nicely!
It will be great to have the full list of arguments in the docs.

Also, I don't know if this is outdated or if any other conversations have happened about thumbnails, but I think that we could name the renders folder to thumbnails as it matches a bit better with the asset-structure-guidelines.md

@beersandrew
Copy link
Contributor Author

It will be great to have the full list of arguments in the docs.

Agreed!

Updated the default folder name, we did discuss this in the wg meeting and I forgot about it, thanks!

@Lucas3Dspain
Copy link

BTW I found the argument in G:\USD_builds\23_11\USD_complete\bin\usdrecord there are some others like

--disableGpu
--renderSettingsPrimPath

This is an extract of the file

    parser.add_argument('--purposes', action='store', type=str,
        dest='purposes', metavar='PURPOSE[,PURPOSE...]', default='proxy',
        help=(
            'Specify which UsdGeomImageable purposes should be included '
            'in the renders.  The "default" purpose is automatically included, '
            'so you need specify only the *additional* purposes.  If you want '
            'more than one extra purpose, either use commas with no spaces or '
            'quote the argument and separate purposes by commas and/or spaces.'))

@meshula
Copy link
Contributor

meshula commented Nov 7, 2023

Excited to see this in action! @beersandrew Could you mark the conversations above as Resolved, wherever there is a "Resolve conversation" button, so the reviewers will know if any thing is still outstanding?

@beersandrew
Copy link
Contributor Author

@meshula Everything is currently resolved now, remaining comments are on the entire PR so I can't resolve them

@meshula
Copy link
Contributor

meshula commented Nov 7, 2023

Ok cool, no worries :)

@jcowles
Copy link
Collaborator

jcowles commented Nov 28, 2023

Great work, Andy!

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