General API Improvements

[BradyAJohnston]

This is an issue to discuss & collect issues around fixing and improving the API and the API nomenclature as user feedback comes in.

[PardhavMaradani]

Hi Brady, I have some feedback about the styles API (as an end user). Should I add them here or file a separate issue? There might be quite a few items, so wanted to check if a separate issue for it makes sense. Thanks

[BradyAJohnston]

@PardhavMaradani maybe detail overview here and we can break out into individual issues for fixing / changing

[PardhavMaradani]

This was one of the pending items on my list to share that I mentioned in this comment of #930.

There are other issues with the current style interface - I will write a separate issue with more details at a later time, ...

I know we had lots of discussions before the current style support was added to the Trajectory entity (following the path of Molecule entity) as part of #879. I am not bringing those up again, but wanted to share something as a user who used the API as well as the GUI quite a bit for testing, adding documentation and creating images and videos for status reports, etc. I'll try to outline what I saw as potential issues along with reasons and maybe add some suggestions.

What I see as issues, not necessarily in any particular order:

• entity.add_style(...) returns the entity object. I understand that this is useful for chaining, etc. I see the value of chaining for stuff that does transformations etc and in cases when there is nothing explicit to return. In this case there is a whole set of style configuration that could be returned which is extremely useful - especially given how the style access is just limited to an index based access (see later). Maybe this is a personal choice. Chaining also makes one wonder if there is some implicit ordering involved / required etc.
• The only way access the object to further configure or update the style is through indexing like entity.styles[0]. Though there is a name we add to the style (both through API and GUI), this isn't accessible and the only way to figure out which style this is, is by looking at the class name (which shows something like ...DynamicStyleInterface_Cartoon... or inferring from the attributes - both of which are not user friendly
• Any index based access is problematic when the values in between can be removed and the whole list re-ordered. The same is the case here. Imagine having three styles, removing the 2nd one (see why remove is needed here because of lack of visibility control, etc later) and then trying to modify the last one through the API. Removal of an individual style is also only through an indexed access entity.styles[i].remove() making this really painful in a background mode when there is no GUI for reference
• There is support for classes like mn.StyleCartoon to create a style object with all the style config that can be passed to add_style. I know this is currently auto-generated, but these classes could be confusing to users. Users might be tempted to think that modifying the values of this object (that was passed to add_style) will somehow update / modify the values. But this class is solely for inputs and thus limited in its utility for only initial creation. This is also entirely different from the returned style interface class
• The returned style interface is limited to the style node inputs. There is no other way to access / modify any of the other inputs passed during the style addition (like name, color, selection, etc). The lack of visibility control also means that the only way to change any of these is to delete and re-create. Consider a simple case of having to adjust the alpha value of a uniform color or update the selection to include an additional resid
• This is an internal detail, but every time the styles attribute of an entity is accessed (entity.styles), a new dynamic styles interface from tree is created. This has a potential to lead to memory issues if used incorrectly
• Because the style interface is based on node inputs and the same are displayed in the GUI, the hints just show Input value used for unconnected socket.. This is just a Blender issue as I mentioned in this comment of PR Add style branches support for Trajectories #888 - see Blender feature request and others experiencing the same

Some of my recommendations for these would be:

• Maybe use something like a styles manager for entity.styles that provides name based subscriptable, iterable and index based access to added styles (similar to Blender collections and entity.annotations). This might also help with not having to create multiple interface objects to return etc
• Maybe add explicit style creation methods like entity.styles.add_cartoon(...), .add_spheres(...), etc that provide access to all the inputs as optional args. These could either be dynamically generated or the signatures created dynamically so they help with auto-complete, docs and also remain up-to-date even with the corresponding style node changes - returning the style interface object for further configuration
• This is not necessarily with the API, but with internal implementation, but we need a way to expose values beyond the node inputs
• Maybe use Blender properties (could be dynamically generated) in the interface that bridge between the API/GUI and the nodes - this would also fix the hints issue and maybe help with exposing the other missing inputs today like name, color, selection, etc. We obviously cannot do dynamic node creations, linking, updates etc from here because of the crashes we know about when these properties are animated

[BradyAJohnston]

Thanks for the detailed write up. Over the past week or two of using it more heavily I have come to some similar conclusions as well. I've got some somewhat scattershot thoughts about different parts, but I think we are mostly aligned that how it currently works isn't ideal and needs some rethinking.

I think what would solve a lot of these issues is if the style classes which currently just act as a way to pass parameters during setup, were linked somehow to the specific molecule / style branch / node.

s = mn.StyleCartoon()
mol.styles.add(s)
s.quality = 4

During creation if we store reference to a specific node (the style node), then we can infer everything else that is needed. Some of the parameters are just stored initially, but are "realised" once applied to a node entity / node tree and any potential named attributes etc are created.

entity.add_style(...) returns the entity object.

I could see this changing or even being removed entirely. Rather than having specific methods for each style, we could just have an .add() that takes the style classes.

mol.styles.add([
    mn.StyleCartoon(), 
    mn.StyleSpheres(selection="is_side_chain"), 
    mn.StyleSurface(selection="is_peptide", material=mn.material.AmbientOcclusion())
])

The only way access the object to further configure or update the style is through indexing

Certainly the dynamic creation of interfaces I think isn't great. The biggest problem like you said is that it is an unknown what the API for it will be until it is actually created. This isn't great IDE tab auto-complete or hints etc.

I think I've come to the conclusion that at least in terms of a python API, we shouldn't be trying to support different potential node setups and should instead just try to expose the settings for a single style node via the API.

The returned style interface is limited to the style node inputs. There is no other way to access / modify any of the other inputs passed during the style addition (like name, color, selection, etc). The lack of visibility control also means that the only way to change any of these is to delete and re-create. Consider a simple case of having to adjust the alpha value of a uniform color or update the selection to include an additional resid

We just need some more advanced getter and setting functions for this. The getters I am unsure what should be done because currently I just throw an error if you try to get the value or something with a node input. To return something else would include returning the actual node that is plugged, or maybe the string for the named attribute if so. Unsure how to best handle it.

This is not necessarily with the API, but with internal implementation, but we need a way to expose values beyond the node inputs

Unsure what you are meaning with the last two suggestions.

[PardhavMaradani]

I think what would solve a lot of these issues is if the style classes which currently just act as a way to pass parameters during setup, were linked somehow to the specific molecule / style branch / node.

I could see this changing or even being removed entirely. Rather than having specific methods for each style, we could just have an .add() that takes the style classes.

Starting off with the style classes as inputs that get filled in and linked seems convoluted to me. There are no validations that can happen when the styles input class is created (what do the selection phrase / attribute or other input types get validated against?) The whole deferred nature of these might make these very messy later. I still don't see what the issue is with simple and clean explicit API calls for these styles. Unlike annotations, which are dynamic and user extensible, these are static, so maybe simpler the better?

I think I've come to the conclusion that at least in terms of a python API, we shouldn't be trying to support different potential node setups and should instead just try to expose the settings for a single style node via the API.

This would require everything that's configurable to be part of a single style node and goes back to the discussions in #879 - stuff like visiblity, color, selection, etc.

Unsure what you are meaning with the last two suggestions.

The returned interface cannot be solely based on the node inputs (as it is today). There is no way to handle support for a customizable selection phrase in this model - to do that, we'd need a way to get an update callback when that changes (both from API and GUI - the GUI currently links to the node inputs, so there is no callback mechanism) where we can do validations, update the corresponding named attribute etc. So, it has to be Blender properties for at least some of the inputs. By values beyond the node inputs, I meant at least having support for name, visibility, color and selection control (ideally, style type too because that's independent of the inputs).

[BradyAJohnston]

After thinking about it some more I think I agree with what you are proposing @PardhavMaradani - that rather than passing in a dataclass with settings, it should just be a method on the entity's style managed. Given each entity will have different potential styles it would remove any confusion about passing StyleSpheres() into a density object or similar.

traj.styles.add_spheres(...)
traj.styles.add_cartoon(...)

density.styles.add_surface(...)
density.styles.add_wireframe(...)

These methods for the style manager would have to remain auto-generated from the nodes inside of the .blend file - as we don't want to be manually updating every time something is changed on the styles themselves. There will likely be quite a few changes for 5.0.

If each entity's style manager inherits from the base StyleManager which we manually code but the the specifics with their particular methods are auto-generated I think that would work.