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

get_prop_name #4475

Open
Durman opened this issue May 16, 2022 · 19 comments · May be fixed by #4515
Open

get_prop_name #4475

Durman opened this issue May 16, 2022 · 19 comments · May be fixed by #4515

Comments

@Durman
Copy link
Collaborator

Durman commented May 16, 2022

Problem statement

sverchok/core/sockets.py

Lines 349 to 364 in 89b770b

def get_prop_name(self):
"""
Intended to return name of property related with socket owned by its node
Name can be replaced by twin property name in draft mode of a tree
If does not have 'missing_dependency' attribute it can return empty list, reasons unknown
"""
node = self.node
if hasattr(node, 'missing_dependency'):
return []
if node and hasattr(node, 'does_support_draft_mode') and node.does_support_draft_mode() and hasattr(node.id_data, 'sv_draft') and node.id_data.sv_draft:
prop_name_draft = self.node.draft_properties_mapping.get(self.prop_name, None)
if prop_name_draft:
return prop_name_draft
return self.prop_name
return self.prop_name

In a test tree execution of this method takes 1% of whole time. It seems that it's probably too expensive for just getting name of a default value. I'm not sure which solution can be proposed here.

@zeffii
Copy link
Collaborator

zeffii commented May 17, 2022

can it be cached? the value isn't likely to change often on any given socket.

1% improvement might also be a quest with diminishing returns

@zeffii
Copy link
Collaborator

zeffii commented May 17, 2022

cant this be written a bit simpler?

if node and hasattr(node, 'does_support_draft_mode') and node.does_support_draft_mode() and hasattr(node.id_data, 'sv_draft') and node.id_data.sv_draft:

vs

if node and getattr(node, 'does_support_draft_mode', False) and getattr(node.id_data, 'sv_draft', False):

?

@Durman
Copy link
Collaborator Author

Durman commented May 17, 2022

I don't think this can be catched because it should return different values dependently on sv_draft mode of a tree. It seems solution can be to replace self.prop_name on change of the sv_draft mode with name of a sv draft property.

@Durman
Copy link
Collaborator Author

Durman commented May 18, 2022

self.node is expensive

image

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

in certain "high volume" situations we could use a cache, as socket instances will never be shared by multiple nodes.

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

this could be worse :)

_highspeed_NodeSocket_Origins = {}    

    def get_prop_name(self):
        """
        Intended to return name of property related with socket owned by its node
        Name can be replaced by twin property name in draft mode of a tree
        If does not have 'missing_dependency' attribute it can return empty list, reasons unknown
        """
        node = _highspeed_NodeSocket_Origins.get(self.socket_id)
        if not node:
            node = self.node
            _highspeed_NodeSocket_Origins[self.socket_id] = node
        
        if hasattr(node, 'missing_dependency'):
            return []

        if node and getattr(node, 'does_support_draft_mode', False) and getattr(node.id_data, 'sv_draft', False):
            prop_name_draft = node.draft_properties_mapping.get(self.prop_name, None)
            if prop_name_draft:
                return prop_name_draft

        return self.prop_name

@Durman
Copy link
Collaborator Author

Durman commented May 18, 2022

Hm, it might give some speed up but looks quite complicated.
Probably such thing could be implemented as separate property

Class Socket:
    cache = {}  # should be reseted on undo events etc.

    @property
    def node_(self):
        if self.socket_id not in Socket.cache:
            Socket.cache[self.socket_id] = self.node
        return Socket.cache[self.socket_id]

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

yes! suitable.
also is there a genuine need to check node here?

        if node and getattr(node, 'does_support_draft_mode', False) and getattr(node.id_data, 'sv_draft', False):
            prop_name_draft = node.draft_properties_mapping.get(self.prop_name, None)
            if prop_name_draft:
                return prop_name_draft

        return self.prop_name

vs

        node = self.node_
        if getattr(node, 'does_support_draft_mode', False) and getattr(node.id_data, 'sv_draft', False):
            prop_name_draft = node.draft_properties_mapping.get(self.prop_name, None)
            if prop_name_draft:
                return prop_name_draft

        return self.prop_name

and more and more i get the sense that there is a less verbose way to write this

            prop_name_draft = node.draft_properties_mapping.get(self.prop_name, None)
            if prop_name_draft:
                return prop_name_draft

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

maybe i'm missing some logic here but..

     def get_prop_name(self): 
         """ 
         Intended to return name of property related with socket owned by its node 
         Name can be replaced by twin property name in draft mode of a tree 
         Currently the presence of the 'missing_dependency' attribute indicates a degenerate node.
         """ 
         node = self.node_
         if not node or hasattr(node, 'missing_dependency'): 
             return self.prop_name   # '' should be as good as []

         if getattr(node, 'does_support_draft_mode', False) and getattr(node.id_data, 'sv_draft', False): 
             return node.draft_properties_mapping.get(self.prop_name, self.prop_name) 
  
         return self.prop_name 

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

unless we are testing the result of isinstance(self.get_prop_name(), list) somewhere..

@Durman
Copy link
Collaborator Author

Durman commented May 18, 2022

also is there a genuine need to check node here?

It can be None for the same reason when socket.is_linked is False when it should beTrue.

Another solution would be to path node as parameter.

def get_prop_name(self, node): ...

# in a node process method
self.inputs[0].sv_get(self)

It's ugly but super efficient and simple. Potentially it requires a lot of little fixes in code of nodes.

Returning empty list by the method is quite ugly. The name of the method suggests that it should return a string.

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

Potentially it requires a lot of little fixes in code of nodes.

akward : /

@zeffii
Copy link
Collaborator

zeffii commented May 18, 2022

If it's really worth avoiding self.node maybe gradually convert nodes? and make node an optional kw of sv_get ?

def sv_get(self, default=sentinel, deepcopy=True, implicit_conversions=None, node=None)
   ...

and same for def get_prop_name(self, node=None): ?

@Durman
Copy link
Collaborator Author

Durman commented May 18, 2022

I think it worths because it has O(len(tree.nodes)) to call, so in bigger trees (more then 100 nodes) the overhead can be even more significant.

Probably the node parameter should be placed before other ones so it will be easier to convert it into mandatory parameter later.

def sv_get(self, node=None, *, default=sential, deepcopy=True, implicit_conversion=None):
    ...

The asterisk should halpe to remove None default value of the node parameter but it assumes that now all parameters are passed via keywords.

@zeffii
Copy link
Collaborator

zeffii commented May 19, 2022

ok. i think the vast majority of nodes can be updated with a relatively simple search/replace, the remaining 20/30 nodes we can do by hand.

@Durman
Copy link
Collaborator Author

Durman commented Jun 3, 2022

I think it's possible simplify the draft mode and instead of calling the properties_mapping method to use a naming convention:

if socket.id_data.sv_draft:
    return f"{socket.prop_name}_draft"
else:
    return socket.prop_name

But this seems can break layouts if some nodes do not use this convention now and their draft properties should be renamed to fit. And this does not take into account that not all nodes have draft properties 🤔

Probably this can be done in the sv_get method instead

if socket.id_data.sv_draft:
    try:
        return getattr(node, f"{socket.prop_name}_draft")
    except AtrributeError:
        return getattr(node, socket.prop_name)
else:
    return getattr(node, socket.prop_name)

Looks a bit overcomplicated but might be more efficient.

I have looked at the code and it seems all nodes follow the convention except the Number node which instead of _draft suffix has draft_ one. 🔫

Probably breaking layouts in draft mode is not we must afraid of.

@Durman Durman linked a pull request Jun 8, 2022 that will close this issue