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

"Bevel a Curve (Surface)" node et al #4268

Merged
merged 20 commits into from
Aug 8, 2021
Merged

"Bevel a Curve (Surface)" node et al #4268

merged 20 commits into from
Aug 8, 2021

Conversation

portnov
Copy link
Collaborator

@portnov portnov commented Aug 7, 2021

  • "Taper & Sweep" node gets a "NURBS" mode. In this mode, it will process only NURBS curves, and output NURBS surface.
  • Old "Bevel a Curve" node is renamed to "Bevel a Curve (Mesh)"
  • New node is introduced: "Bevel a Curve (Surface)". In general, it is similar to "Bevel a Curve (Mesh)", but instead of working with mesh objects, it takes Curve objects and outputs a Surface object. Depending on settings, it can either work with arbitrary Curve objects and generate a generic Surface object, or work with NURBS curves and output a NURBS surface.

Screenshot_20210807_123146
Screenshot_20210805_214517
Screenshot_20210807_221243
Screenshot_20210807_223851
Screenshot_20210807_223550
Screenshot_20210807_225058

Preflight checklist

Put an x letter in each brackets when you're done this item:

  • Code changes complete.
  • Code documentation complete.
  • Documentation for users complete (or not required, if user never sees these changes).
  • Manual testing done.
  • Unit-tests implemented.
  • Ready for merge.

@portnov portnov added the NURBS label Aug 7, 2021
Comment on lines +195 to +196
if not any(socket.is_linked for socket in self.outputs):
return
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need this check now. It makes update system more complex. It can be even less efficient because some nodes update even when there are no output links but update system have to update them anyway. Update system does not know whether the node has this check or no.

# this is only because some nodes calculated data only if certain output socket is connected
# ideally we would not like ot make previous node outdated, but it requires changes in many nodes
if not has_old_from_socket_links:
link.from_node.is_input_changed = True
else:
link.to_node.is_input_changed = True

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid I don't understand.
If the node does not have connected outputs, why spend time doing complex calculations?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some nodes does not have such optimization. How update system should understand whether a node has this check or not to update the node properly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand, the only case when the node has to be updated without having connected outputs - is the case when it has "side effects", such as drawing in the viewport or generating objects in scene. We have only few of such node classes.
So, such nodes could have special class property (has_side_effects = True), so that update system could recognize them and trigger their update even when they do not have connected outputs.
For other nodes, update should not be triggered when they do not have connected outputs.
If we will have such logic in update system, we can remove the check from the nodes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But for now, do I correctly understand that the update system triggers process() of the node even if does not have connected outputs?
In this case nodes which do complex calculations have to do the check themselves.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes the process is called even there are no output connection of a node. I guess there are case when such update are expected for example SNL node could do something without passing any result to next nodes.
The thing is that such checks do not let make optimization in update system and adding this checks in new nodes make future improvements in this direction harder.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But without this check node trees which include complex nodes do excessive computations. And at least for some nodes it means a lot of time.

@portnov portnov merged commit b5524f4 into master Aug 8, 2021
@portnov portnov deleted the bevel_surface_node branch August 8, 2021 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants