Page MenuHome

Edit Voxel Size: Clamp the text position to the viewport bounds when not visible
Needs ReviewPublic

Authored by Pablo Dobarro (pablodp606) on Jun 18 2020, 6:57 PM.

Details

Summary

The text used to be rendered on the center of the preview grid. When
using the operator too close to a model, it is possible to have the
center of the grid off screen, so it was not visible. In those
situations, the position is clamped so it never goes outside the view, fixing
that issue.

Diff Detail

Repository
rB Blender
Branch
edit-voxel-size-text-center (branched from master)
Build Status
Buildable 8650
Build 8650: arc lint + arc unit

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Jun 18 2020, 6:57 PM
Pablo Dobarro (pablodp606) created this revision.

Consider moving to an utility function, which will take care of giving the final position. The function is already too long, no need to cam everything possible into it.

@Julian Eisel (Severin), how do you fee about using the center? I think more common behavior is to "clamp" to the corner/edge closest to the "pivot".

source/blender/editors/object/object_remesh.c
604

Remove debug print.

Pablo Dobarro (pablodp606) marked an inline comment as done.
  • Review Update

From code implementation aspect seems fine.

I'd leave the final decision on the opinion about magnet-to-corner vs. jump-to-center decision up to the UI team.

Marking as accepted from my side. I can live with any UI decision.

This revision is now accepted and ready to land.Jun 22 2020, 11:43 AM

Indeed, I have to agree with Sergey here.
The position feels very random to me from looking at the video, it didn't seem predictable. I had to read the patch/description and carefully rewatch to understand the positioning, even then it wasn't really predictable.

This is already an improvement though, it's just that having it clamp to the visible bounds would be much preferable. Similar to how popups are clamped at screen edges.

  • Clamp the text position instead of using the area center

Pablo Dobarro (pablodp606) retitled this revision from Edit Voxel Size: Move the label to the center of region when not visible to Edit Voxel Size: Clamp the text position to the viewport bounds when not visible.Sep 18 2020, 1:48 AM
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)
Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)

@Julian Eisel (Severin) The position is clamped now instead of moved to the center. Is this ok?

This patch has an approved status however it was not committed yet. So I'm assuming the other reviewers are blocking. Updating the reviewers list to reflect this. This way the patch status still show as Need Review.

This revision now requires review to proceed.Mar 26 2021, 5:54 PM