Page MenuHome

Fix T100491: Mouse selection is inaccurate in NLA Editor
ClosedPublic

Authored by Richard Antalik (ISS) on Aug 19 2022, 7:12 AM.

Details

Summary

Selection range is +/-7 pixels to actual clicked position, but strip selection
was biased towards rightmost strip.

To make selection more intuitive, select closest strip to clicked position, and
stop iterating when strip intersects clicked pixel.

Diff Detail

Repository
rB Blender
Branch
arcpatch-D15728 (branched from master)
Build Status
Buildable 24594
Build 24594: arc lint + arc unit

Event Timeline

Richard Antalik (ISS) requested review of this revision.Aug 19 2022, 7:12 AM

Good change, I would just suggest to add a comment to explain that block since it isn't immediately obvious what it's doing.

source/blender/editors/space_nla/nla_select.c
301–308

I would defer this calculations until after the BKE_nlastrip_within_bounds returns true.

Good change, I would just suggest to add a comment to explain that block since it isn't immediately obvious what it's doing.

Will modify that, also realized, that BKE_nlastrip_within_bounds(strip, (int)mouse_x, mouse_x) wont work if you manage to click on exact frame value, so this will need own function probably.

Add comments and implement explicit check for strip-frame intersection

Richard Antalik (ISS) marked an inline comment as done.Sep 6 2022, 1:17 PM
Sybren A. Stüvel (sybren) requested changes to this revision.Nov 8 2022, 11:04 AM

I think overall the approach is ok, but I have the feeling there are more computations done than necessary. How about changing BKE_nlastrip_intersects_frame() to float BKE_nlastrip_distance_to_frame(strip, frame) that returns the actual distance of the frame number to the strip itself? I'm thinking something like (untested and badly formatted):

float BKE_nlastrip_distance_to_frame(const NlaStrip *strip, const float timeline_frame) {
  if (timeline_frame < strip->start) return strip->start - timeline_frame;
  if (strip->end < timeline_frame) return timeline_frame - strip->end;
  return 0.0f;
}

You can then use this distance to find the closest, and when distance == 0.0f you're sure that it intersects -- no further processing necessary.

This also provides a better separation of abstraction -- rather than having the loop itself doing some computations and deferring others to a separate function, with my suggested approach only one function is responsible for computing that distance.

As a request for future passes, before submitting please do a pass to verify that all parameter and local variables that can be const actually are declared as such.

source/blender/blenkernel/BKE_nla.h
283

This should document whether the NLA strip is seen as open, closed, or half-open interval.

source/blender/blenkernel/intern/nla.c
1363

const float

1365

I wouldn't check for NULL here. The function queries for a property of a strip, which is an invalid operation if there is no strip. This could even be hiding bugs in calling code. I'd recommend using BLI_assert_msg(strip != NULL, "NULL strip cannot be checked for intersection") instead.

1365

For the frame number comparisons, use a smaller-to-bigger ordering so that each comparison uses the same operator:

strip->start < timeline_frame && timeline_frame < strip->end

Also I think that the comparison is incorrect, and that it should use <= instead of <.

1365–1369
if (X) return true;
else return false;

is exactly the same as:

return X;
source/blender/editors/space_nla/nla_select.c
290

const

302

const

This revision now requires changes to proceed.Nov 8 2022, 11:04 AM
Richard Antalik (ISS) marked 6 inline comments as done.
  • Address inlines

Sorry for non-constness, normally I sculpt code instead of writing, so ensuring constness is discrete cleanup pass for me. Here I forgot to do it :/

source/blender/blenkernel/intern/nla.c
1365

I wouldn't check for NULL here. The function queries for a property of a strip, which is an invalid operation if there is no strip. This could even be hiding bugs in calling code. I'd recommend using BLI_assert_msg(strip != NULL, "NULL strip cannot be checked for intersection") instead.

Agree in general(re: https://devtalk.blender.org/t/some-anti-patterns), but since I don't know this area well, I looked around to see if this is general practice to do these checks. Probably would have been able to deduce if this is necessary, but such information may be hidden behind context abstractions and my intention was very quick and simple fix. Not sure if you want to form a guideline and whether this would be scenario that should be documented. Also sorry for responding on wrong platform :) When I read post, my reaction was I agree and pretty much don't have much to add.

1365

Will do as you wish here. In VSE I pay quite a bit of attention to form these comparisons, so they can be read in a way, that is most natural. If that can't be done I often add comment to explain in plain language.

Sybren A. Stüvel (sybren) requested changes to this revision.Nov 10 2022, 11:54 AM

... my intention was very quick and simple fix
In VSE I pay quite a bit of attention to form these comparisons ...

From the in-line comments, I get the impression that you're more careful with the VSE code than with the NLA code. That's a bit of a dubious attitude, and I hope I'm interpreting things incorrectly.

Anyway, the patch looks almost good to go. Just a little nag about the documentation of BKE_nlastrip_distance_to_frame().

source/blender/blenkernel/BKE_nla.h
283

I don't think the concept of "nearest distance" exists. "Nearest" already depends on a distance metric itself.

How about this?

Return the distance from the given frame to the NLA strip, measured in frames. If the given frame intersects the NLA strip, the distance is zero.

This revision now requires changes to proceed.Nov 10 2022, 11:54 AM
Richard Antalik (ISS) marked an inline comment as done.
  • Clarify comment.

From the in-line comments, I get the impression that you're more careful with the VSE code than with the NLA code.

That is incorrect. I have said, that here I wrote statement in a way I would have done in VSE. It would be pretty mad to say "I don't care about this editor, lets improve it" :)

That is incorrect. I have said, that here I wrote statement in a way I would have done in VSE. It would be pretty mad to say "I don't care about this editor, lets improve it" :)

Yup, I'm happy to have been wrong here :)

Just one small note, no need to re-review after addressing it.

source/blender/blenkernel/BKE_nla.h
286

Remove the const here, it has no meaning in a function declaration for pass-by-value types.

This revision is now accepted and ready to land.Nov 11 2022, 12:56 PM