-
Notifications
You must be signed in to change notification settings - Fork 19
Add Version Info (Author, Date, Status) fields to generic loader #340
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
base: develop
Are you sure you want to change the base?
Add Version Info (Author, Date, Status) fields to generic loader #340
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! I like it - I'm just slightly worried about how this may affect performance with the additional queries - but overall it should be fairly limited due to the caching - so maybe less of a worry.
Should we also include the version comment as multiline parm? :)
client/ayon_houdini/api/hda_utils.py
Outdated
| @lru_cache(maxsize=100) | ||
| def _cached_get_representation_by_id(project_name: str, representation_id: str) -> dict: | ||
| return get_representation_by_id(project_name, representation_id) | ||
|
|
||
| @lru_cache(maxsize=100) | ||
| def _cached_get_version_by_id(project_name: str, version_id: str) -> dict: | ||
| return get_version_by_id(project_name, version_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a reason why I didn't lru_cache the other things and stored them on the session instead 🤔 but can't think of it now.
To reduce the size of the cached data, it may be worth calling these with fields={} argument where we're explicit about which fields we actually want to query.
E.g. it may be worth avoiding getting in the massive files list from representations with all the site information and the context if both are not used in our use case.
Also, looking at our use case it may be worth calling it even get_version_info and if no entity is found return an empty dict so that the type hint is consistent on the return value.
And the HDA then can just do get_version_info().get("author", "")
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the lru_cache and changed it to use the session_cache instead to align with the existing codebase.
I noticed that the session cache is a bit more persistent. For example reloading the hda_utils module doesn't clear it, which was the case for the lru_cache.
I also changed the functions a bit to expression_get_version() and expression_get_representation() which each return a cached dict of the respective entities that can then be used in the parm expressions:

I'm still trying to figure out whats the best layout for caching these calls.
currently all three: get_version_info, get_representation_info and the existing expression_get_representation_path use their own cache, while other functions such as get_version_formatted_time rely on the calling these cached functions.
I'd like to see if we can incoperate the expression_get_representation_path to reuse the same cache as get_representation_info and/or if its worth caching the entity id's separate. for example the get_representation_info uses at least 2 API calls: first to resolve the inputs and then to call get_representation_by_id. But we're only using the id from the info result.. so that second call could be omitted.
...ini/startup/otls/ayon.generic_loader.hda/ayon_8_8Object_1generic__loader_8_81.0/DialogScript
Outdated
Show resolved
Hide resolved
| default { [ "from ayon_houdini.api import hda_utils\n\nversion = hda_utils.expression_get_version_entity()\nreturn version.get(\"author\") or \"\"" python ] } | ||
| parmtag { "script_callback_language" "python" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be worth avoiding all the explicit imports across all the usages here, and instead import them from the hou.phm() like the others - just make sure to actually import them than into the HDA Python Module, like here:
Line 8 in 5327b40
| select_product_name, |
| parm { | ||
| name "nodes_referencing_file" | ||
| label "Nodes Referencing File" | ||
| type oplist | ||
| default { [ "\" \".join(parm.node().path() for parm in hou.parm('./file').parmsReferencingThis())" python ] } | ||
| parmtag { "oprelative" "/" } | ||
| parmtag { "script_callback_language" "python" } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MustafaJafar how do you feel about moving this here instead of keeping it where it was?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a couple of other minor suggestions regarding the ui layout which are related to this.
Since this PR is introducing more "info" parameters I figured I'd move the "settings" into their own subfolder.
I'm happy to split those changes off into their own PR so we can discuss them separate.
quick preview of my local version:
Screencast_2025-12-22T17.42.07.mp4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MustafaJafar how do you feel about moving this here instead of keeping it where it was?
tbh. I don't have a strong opinion on that.
But, personally, I'd like to have Nodes Referencing File at the end ungrouped.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a couple of other minor suggestions regarding the ui layout which are related to this.
Since this PR is introducing more "info" parameters I figured I'd move the "settings" into their own subfolder. I'm happy to split those changes off into their own PR so we can discuss them separate.
quick preview of my local version:
Screencast_2025-12-22T17.42.07.mp4
I like version and representation entity ids with the exclamation mark icon. and, also the display settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a couple of other minor suggestions regarding the ui layout which are related to this.
Since this PR is introducing more "info" parameters I figured I'd move the "settings" into their own subfolder. I'm happy to split those changes off into their own PR so we can discuss them separate.
quick preview of my local version:
Screencast_2025-12-22T17.42.07.mp4
Nice - I quite like it.
I'd love for the Nodes Referencing File to still be visible without needing to expand it - so that it's right there directly.
I'm fine with e.g. the top Info section being expanded by default, which I think is the group_default tag set to 1 on the parm?
I'm not too sure about the Info > Info headers nesting. I'd rather have them be distinctly named. Some ideas:
Info > Metadata
Info > Entity Data
Also, I think we should nudge the Loader section in the Info bar to the bottom. It's the least informative section, or at least the one the majority would not look at.
So I'm thinking:
Info
Usage
Metadata
Loader
I quite like the Descriptive Parm too. May be best left to a follow up PR instead of flooding this one.
Actually - thinking about it now. Then the other parms could just reference the entity parms to display whatever they want, like the author, etc. by default. But it'd also make it trivial for anyone to add another parm for displaying of e.g. custom attributes or data on the entities? |
…on_info and use those to calculate parm values
|
Just want to say I really like this PR. Thank you @vincentullmann. |
| return False | ||
|
|
||
|
|
||
| def expression_get_representation_id() -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I removed it when I changed the expression to get_representation_info().get("id").
It felt cleaner to use get_...info() calls across the board instead of different methods for each parm.
The overall goal would be to update all types of generic loaders before merging. So far I've only updated the Obj-variant to avoid having to reapply the same changes multiple times while iterating on this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is likely some users might have edited versions or nodes with edited expression in their scenes which rely on the previous function calls, so it is probably a good idea to bring it back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove it after updating it in all of the HDA variants.
Not sure if we want to keep it afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Refactor all and cleanup what's not needed anywhere.
| product_name, | ||
| version, | ||
| ) | ||
| return get_version_by_id(project_name, version_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are best reversed, or the latter should not call the first. Now you're querying the database twice.
But the other way you don't with:
get_version_id(...):
get_version(...)["versionId"]There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nevermind - that's wrong because the resolve endpoint doesn't return the actual entity, just the relevant ids 🤔
| parm { | ||
| name "info_author" | ||
| label "Author" | ||
| type string | ||
| default { [ "hou.phm().get_version_info().get(\"author\") or \"\"" python ] } | ||
| parmtag { "script_callback_language" "python" } | ||
| } | ||
| parm { | ||
| name "info_date" | ||
| label "Date" | ||
| type string | ||
| default { [ "hou.phm().get_version_formatted_time()" python ] } | ||
| parmtag { "script_callback_language" "python" } | ||
| } | ||
| parm { | ||
| name "info_status" | ||
| label "Status" | ||
| type string | ||
| default { [ "hou.phm().get_version_info().get(\"status\") or \"\"" python ] } | ||
| parmtag { "script_callback_language" "python" } | ||
| } | ||
| parm { | ||
| name "info_comment" | ||
| label "Comment" | ||
| type string | ||
| default { [ "hou.phm().get_version_info().get(\"attrib\", {}).get(\"comment\") or \"\"" python ] } | ||
| parmtag { "script_callback_language" "python" } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we prefix the info parms with AYON_, e.g. AYON_info_comment? @MustafaJafar thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for the AYON_ is because of the possible conflict on ROP nodes. but this HDA won't need it since it's an AYON HDA anyways.
Thanks @vincentullmann - regarding this... I think maybe getting the values from a JSON string parm is worse than using cached data UNLESS somehow that json string could persist in the scene file if e.g. the server loses connection or you're in offline mode - that way the metadata is accessible in the file itself. However, that's now how houdini works with expressions. The issue with the JSON string is just the fact that now every other parm needs to parse a JSON string, just adding a redundant overhead. I guess it makes just as much sense to allow the other parms to just call e.g. |


Changelog Description
This PR adds some additional info parameters on the generic loader to show the versions author, updated date and status.
Additional review information
The same changes will have to be copied to the SOP and LOP variants of the node. I've not done that yet as I wanted to wait for a first round of feedback / discussion about the change and implementation.
Testing notes:
Demo:
Screencast_2025-12-20T00.53.16.mp4