Page MenuHome

Collapsed nodes don't shrink
ClosedPublic

Authored by Robert Guetzkow (rjg) on Mar 29 2019, 10:08 PM.

Diff Detail

Repository
rB Blender

Event Timeline

Superb. This is so much nicer I think.

To me it seems like this does exactly the right thing.

Before:

After:

This revision is now accepted and ready to land.Mar 29 2019, 11:19 PM
Brecht Van Lommel (brecht) requested changes to this revision.EditedMar 29 2019, 11:36 PM

Just changing it for display will break the resizing operator, it will not match.

I think miniwidth could be eliminated entirely, it's not clear to me why collapsed nodes should have a different width.

This revision now requires changes to proceed.Mar 29 2019, 11:36 PM

What is the expected behavior for resizing? I assume it should be possible to resize both in open and collapsed state and the size should remain consistent except for when the open state is too small for the circular arrangement in collapsed state. Then the node in collapsed state may be larger (the minimum width necessary to draw both circular sides without overlap).

  • Resizing results in same size between open and collapsed node, except when the collapsed node needs to be wider due to the rounded caps
  • miniwidth usage is removed

I can increase the minimum width for the collapsed state, if you think that it's too short.

To me it seems to work well - it does pretty much exactly what I'd expect - the nodes keep the correct width when you collapse them:

If you elongate a node and then collapse, the collapsed state still inherits the new width, which I think is correct:

And it seems to work well with 'mega-nodes' too:

So I would just say, from a functionality perspective it works great!

@Brecht Van Lommel (brecht) ?

Jacques Lucke (JacquesLucke) requested changes to this revision.Apr 2 2019, 2:08 PM

Only some minor issues. After those small cleanups, the patch looks good to me.

source/blender/editors/space_node/node_draw.c
561

I'd prefer to slowly move away from declaring all variables at the top. Better try to minimize the vertical scope of a variable.
Also a better name might just be min_hidden_width. Or maybe this variable is not necessary at all with the change below.

588

max_ff

1181

minor indentation issue (use spaces for "extra" indentation).

This revision now requires changes to proceed.Apr 2 2019, 2:08 PM
  • Removed variable and used value directly in max_ff
  • Proper indentation
Robert Guetzkow (rjg) marked 2 inline comments as done.Apr 2 2019, 2:59 PM

Indentation is still not fixed:

However, I'll fix it before I merge it.

source/blender/editors/space_node/node_edit.c
896 ↗(On Diff #14522)

I found another minor issue, but that does not seem worth the hassle for now.
The problem is that node->typeinfo->minwidth does not contain the real minimum width when the node is collapsed.
This is due to the clamping in the drawing function.

@Jacques Lucke (JacquesLucke) Sorry. For some reason the tab to space conversion was disabled in VS.

@Jacques Lucke (JacquesLucke) Sorry. For some reason the tab to space conversion was disabled in VS.

I recommend you to set "editor.renderWhitespace": "all" in vs code. That makes it easier to see these issues.

Is the value clamping of minwidth of practical relevance, e.g. doing lots of collapses shrinks the node?

I recommend you to set "editor.renderWhitespace": "all" in vs code. That makes it easier to see these issues.

Yes I should've done that. Thanks for the tip.

  • Proper indentation (now for real ;) )
This revision was not accepted when it landed; it landed in state Needs Review.Apr 2 2019, 4:45 PM
This revision was automatically updated to reflect the committed changes.