prman-pixar / RenderManForBlender

RenderMan for Blender render addon
MIT License
808 stars 133 forks source link

Closes #566 #567

Closed adminradio closed 6 years ago

adminradio commented 6 years ago

This fix solves three problems if animation for external render was enabled:

Tested on Windows only, but I don't expect any platform specific issues, anyway testing on linux and macOS is very welcome.

bsavery commented 6 years ago

So the problem with 21.7 is that the property name is > 64 characters? It strikes me that switching to _uio vs ui_open is a temporary fix, which may get people over the hump, but might not be a long run solution. Not a reason not to do it. Just something to keep in mind.

prman-pixar commented 6 years ago

Why is there are property name limit of 64 characters? This fix seems like a hack? Is there a better, less temporary solution to fix this problem than the shorting applied here?

jdent02 commented 6 years ago

The 64 character limit is on Blender's side and relates to how long a property name can be. As stated when the property names are dynamically created (based on information contained in the Renderman .arg files) they are exceeding that limit, causing the initialization of the addon to fail. Short of changing the .arg files to reduce the length of these properties the fix will have to come from Blender's side.

bsavery commented 6 years ago

I think the @prman-pixar point is that if next PRMan release there is another arg parameter name which ends up being super long this problem will still be there. Maybe there could be a test if "prop_name" is > 64 and shortened for the internal prop id name but the external facing label would still be there in long form.

adminradio commented 6 years ago

I'm aware that this may look like a temporary fix or a hack and the limit is indeed on the Blender side and can't be changed. But shorten only a few property names if they exceed a limit, is in my opinion more inconsistant and error prone because you have two different suffixes for the same usage. So, I decided to change all to a shorter one.

Yes - with new arg files, this may become again insufficient in the future. A better solution would be our own property class, that implements some kind of uuid as name and handling of visible/readable text values as it's own properties. That's not an easy task (at least for me). But with RenderMan 22 be on the horizon with an eventually complete rewrite/refactoring of the addon the former solution looks legit to me.

I may overlook something. If there are better ideas, please give me hint - I'll try my best. :)

adminradio commented 6 years ago

@bsavery if we add _ui_open to a property name, then it's always an internal one.

bsavery commented 6 years ago

^ I agree with What Tim said above. And would add a small check to make sure the length is < 64 when creating the prop_name + _ui_open name