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

fix #4723 #4762

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

satabol
Copy link
Collaborator

@satabol satabol commented Nov 14, 2022

fix #4723

now:

image

@Durman
Copy link
Collaborator

Durman commented Nov 15, 2022

Probably it's better to renames sockets instead (not easier though). Because such changes can potentially break layouts.

@satabol
Copy link
Collaborator Author

satabol commented Nov 15, 2022

@Durman But mode Edges->Is Border has an order socket functions like Faces->Is Border. The orders are same now. May be keep the order in Faces->Is Border is better?

@Durman
Copy link
Collaborator

Durman commented Nov 16, 2022

Without creating new version of the node I don't see too much choice but to rename sockets (even if order of names will be inconsistent with names in other modes). Or we can accept your proposal and hope that it wont break any layouts (I usually use Mesh Filter for the operation).

@satabol
Copy link
Collaborator Author

satabol commented Nov 16, 2022

@Durman Sorry. I did second commit accidental ))). I use Mesh Filter too. May be anybody saw that error and did not using it at all? When I was writing docs for that node I did not see that moment too and used mask for examples. I have not strong opinion for that situation.

@satabol
Copy link
Collaborator Author

satabol commented Aug 24, 2023

@Durman

  1. New version of node get no effect because problem inside external function and not in the node. I test if apply changes in code this will effect immediately after reload sverchok without any glitches in existing nodes.
    image

  2. Rename sockets is not good idea because sockets names defined externally bot not inside node.

    'Is Boundary': (70, 'vp', '', '', pols_is_boundary, 'sss', 'Mask, Boundary, Interior', 'Is the face boundary'),

    So if create new version of node and rename sockets in this line then this will affect for both nodes - old and new nodes.

So I propose swap params in this pull request.

@Durman
Copy link
Collaborator

Durman commented Aug 25, 2023

If understand correctly your fix will swap socket outputs. Output of the first socket will go to the second one and vice versa. It's not good because can break existing layouts potentially. What if to rename outputs instead?

-'Is Boundary':        (70, 'vp', '',   '',  pols_is_boundary,      'sss', 'Mask, Boundary, Interior', 'Is the face boundary'),
+'Is Boundary':        (70, 'vp', '',   '',  pols_is_boundary,      'sss', 'Mask, Interior, Boundary', 'Is the face boundary'),

@satabol
Copy link
Collaborator Author

satabol commented Aug 26, 2023

@Durman Rename outputs (2) will harm any case - for old and new nodes. So swap outputs (1) is less evil. )

@Durman
Copy link
Collaborator

Durman commented Aug 27, 2023

Probably you don't understand something or me. The problem what I see is that when you swap sockets (socket 1 outputs data 2 and socket 2 outputs data 1) they start to output different data in old layouts. It means that in old layouts the node and all dependent nodes output one result. But when you open the layout after the fix the node and all dependent nodes will return another result. Ideally we want trees output the same data for all Sverchok versions.
Renaming sockets does not have such effect. Socket names are just information for users.

@satabol
Copy link
Collaborator Author

satabol commented Aug 27, 2023

@Durman You are right. It is not good idea to rename sockets. But you can't rename sockets now. The core of problem is these sockets output names defined externally of analyzer node. This is very strange! Other nodes has no that feature. It is senseless to create new version of analyzer node. If you want new version of analyzer node you have to define new version of param "faces_modes_dict" and other params too (these params defined externally of analyzer node module). It is very hard to support that code in the future.
Or if to place dependency params and functions for this node from "utils\modules\polygon_utils.py" to the "component_analyzer.py" then it has a sense to create a new version of analyzer node. But now one cannot create a new version "analyzer node" at all.
?

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.

Component Analyzer node
2 participants