UI: Fix nodes sockets alignment
ClosedPublic
Actions

Authored by Alessio Monti di Sopra (a.monti) on Dec 2 2021, 2:25 PM.

Details

Reviewers

Hans Goudey (HooglyBoogly)
Aaron Carlisle (Blendify)

Commits

rB3b965ba10bf6: UI: Fix node socket alignment in some cases

Summary

The patch fixes some misalignments in the nodes' sockets/options recently introduced in 26d2caee3ba0,
while maintaining the original fix for T92268.

The original fix made the top padding always of the same size; while that works when the first row of the adjacent nodes is
Socket | Socket, it doesn't for other more common cases, like Socket | Node Option, where the text results misaligned.

Diff Detail

Repository: rB Blender

Event Timeline

Alessio Monti di Sopra (a.monti) requested review of this revision.Dec 2 2021, 2:25 PM

Alessio Monti di Sopra (a.monti) created this revision.

Harbormaster completed remote builds in B19151: Diff 45575.Dec 2 2021, 2:26 PM

Alessio Monti di Sopra (a.monti) added a reviewer: Hans Goudey (HooglyBoogly).Dec 2 2021, 2:27 PM

Thanks for the patch. Do you mind adding a bit more detail to the description, like why the other fix wasn't correct, and how this fixes it at a high level?

This revision now requires changes to proceed.Dec 6 2021, 5:53 PM

Alessio Monti di Sopra (a.monti) edited the summary of this revision. (Show Details)Dec 6 2021, 7:53 PM

Aaron Carlisle (Blendify) requested changes to this revision.Dec 6 2021, 8:11 PM

Aaron Carlisle (Blendify) added a subscriber: Aaron Carlisle (Blendify).

Aaron Carlisle (Blendify) added inline comments.

source/blender/editors/space_node/node_draw.cc
373–374	These should be constants

make variables constants

Aaron Carlisle (Blendify) accepted this revision.Dec 6 2021, 8:26 PM

Just a picky comment about self-documenting code, the screenshot in the patch description looks good.

source/blender/editors/space_node/node_draw.cc
374	The variable name `inputs_first` seems to refer to the check for whether the list is empty: `node->inputs.first`. In that case `node_has_inputs` might be a better name to keep track of. However, there are some other checks used to assign this value (`!(node->outputs.first \|\| (node->flag & NODE_PREVIEW) \|\| node_options)` that aren't really described. Maybe it would make sense to use a bool variable with a descriptive name for those too?

This revision now requires changes to proceed.Dec 13 2021, 7:18 AM

Alessio Monti di Sopra (a.monti) added inline comments.Dec 13 2021, 10:56 AM

source/blender/editors/space_node/node_draw.cc
374	`inputs_first` is meant to indicate whether the first thing drawn is an input socket. If there is a more descriptive name for that one then the way it's defined should make sense even without using additional bools I think. Not sure what it should be though: `inputs_drawn_first`, `inputs_on_top`, `first_is_input` ?