Skip to content

Fix envelope point navigation getting stuck on a point or skipping points when there are multiple points at the same position. #1243

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

Merged
merged 2 commits into from
May 10, 2025

Conversation

jcsteh
Copy link
Owner

@jcsteh jcsteh commented May 9, 2025

Fixes #1242.

@jcsteh jcsteh requested a review from LeonarddeR May 9, 2025 12:24
@jcsteh
Copy link
Owner Author

jcsteh commented May 9, 2025

@LeonarddeR, I'd love a review on this if you have the time, but I totally understand if you don't. I really tried to come up with a simpler or cleaner way to do this, but I just couldn't. Is this readable? Am I missing a simpler approach? Thanks.

Copy link
Contributor

github-actions bot commented May 9, 2025

…ints when there are multiple points at the same position.
Copy link
Contributor

github-actions bot commented May 9, 2025

@LeonarddeR
Copy link
Collaborator

I will have a look over the weekend.

Copy link
Collaborator

@LeonarddeR LeonarddeR left a comment

Choose a reason for hiding this comment

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

I must admit that this is not the easiest code to read. The most important thing seems to me that it works. That said, could using a random access iterator, just as with MIDI events, improve readability? Or is that a totally wrong way of thinking?

point -= direction;
}
}
GetEnvelopePointEx(envelope, currentAutomationItem, point, &time, &value,
Copy link
Collaborator

Choose a reason for hiding this comment

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

It occurs to me that this extra call is superfluous when the call at line 278/279 doesn't result in a skip. is that correct?

Copy link
Owner Author

Choose a reason for hiding this comment

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

You're quite correct. I originally thought that there were code paths that can reach here where the time, etc. hasn't been fetched yet. However, I'm now thinking that might be incorrect, so I might be able to move this inside the if block.

@jcsteh
Copy link
Owner Author

jcsteh commented May 10, 2025

Thanks for the suggestion. I spent some time thinking about this just now, but I don't think a random access iterator will help here. There are a few key differences between envelope points and MIDI events. For envelope points:

  1. There are multiple possible start points. If we previously moved to a point at the cursor, then we always want to move (unless we can't). On the other hand, if we didn't previously move to a point at the cursor, we only want to move from the start point if it isn't at the cursor.
  2. We only have one dimension of navigation for envelope points. With MIDI, we have separate commands for notes and chords. For envelope points, next and previous move even through points at the same position.
  3. REAPER has an API call to get the envelope point nearest the cursor. That means we only need to increment (or decrement) once at most. In contrast, with MIDI events, we potentially have to increment many times to find the event we want, which increases the utility (and justifies the complexity) of an iterator.
  4. For envelope points, we want next and previous to move to the point at the cursor if they can't move any further. That is where a lot of the edge cases here come from. We don't do that for MIDI events, though arguably we should; see MIDI Editor: broken the chords preview moving throughby #897. I suspect fixing that will increase the complexity there too.

@jcsteh
Copy link
Owner Author

jcsteh commented May 10, 2025

I'm happy for someone to take a stab at rewriting this if they have a better idea; I'm not particularly fond of this code. That said, I really don't see how I can make this better despite more thinking about it today; I don't think an iterator is going to make the edge cases any simpler, but maybe I'm missing something. I'll merge it for now.

@jcsteh jcsteh merged commit 89b667c into master May 10, 2025
2 checks passed
@jcsteh jcsteh deleted the env branch May 10, 2025 03:28
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Envelope point navigation gets stuck or skips points if there is more than one point at the same position
2 participants