Skip to content
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

Added options to follow sequencer cursor. #39

Merged
merged 2 commits into from Aug 30, 2020

Conversation

Incognito357
Copy link
Contributor

@Incognito357 Incognito357 commented Aug 25, 2020

Options are:

  • None - default behavior, doesn't follow
  • Jump - stays static until the cursor goes off-screen, then it jumps to that point so the cursor is on screen again.
  • Continuous - always keeps the cursor visible, by constantly scrolling, effectively keeping the cursor in the same spot in the center of the screen.

  None - default behavior, doesn't follow
  Jump - stays static until the cursor goes off-screen, then it
         jumps to that point so the cursor is on screen again.
  Continuous - always keeps the cursor visible, by constantly
         scrolling, effectively keeping the cursor in the same
         spot in the center of the screen.
@Incognito357
Copy link
Contributor Author

Hopefully this is a small enough change that can be verified and snuck into the latest release :)

@Incognito357
Copy link
Contributor Author

A few notes:

  • I built and tested this on windows, as well as linux, and it seems to work well.
  • In continuous mode, there does appear to be a slight jitter that's more apparent at higher zoom levels, maybe due to rounding a float, though I'm not certain. It wasn't distracting enough for me to notice it earlier when I was testing, but now that I did notice it, it's more obvious. Still, I don't think it's so distracting as to be a deal-breaker, but I'll look into it to see if there's something I can try (maybe setting the scroll position in the render function is a bad idea?)
  • Panning is not possible while playing in continuous mode, but it is while in jump mode, which may cause undesirable jumping if you pan the cursor off screen. I'm not too sure how best to address that. I could possibly automatically change the setting back to none if you pan too far, but then you'll need to go back into the settings to re-enable it, which seems a bit annoying.

@BleuBleu
Copy link
Owner

BleuBleu commented Aug 26, 2020

Oh Hi again!

Sorry been bombarded by notifications! Cant keep track of what i replied to or not! 😅

I didn't realize you had the pull request ready! As I said, I am very hesitant to add anything at the moment in 0.2.2, I am in closing mode for this version. So id prefer to stick to bug fixes only.

I do have 2 questions tho!

  • What about the piano roll? This is what people seem to as for the most in term of auto-follow. Seems like we only handle the sequencer with this, right?
  • Also, assuming it affects both the sequencer + piano roll, I think a toggle in the toolbar would be better right? Seems like something you may way to change quite often? What do you think? (My unofficial rule for what belongs in the toolbar is stuff that applies to more than 1 of the UI component, sequencer, piano roll, etc.).

EDIT: Also, the code that follow should not be in the Rendering code, but rather in the Tick() code. Rendering code should stick to simply drawing.

EDIT2: Actually, ill take a crack a it on Friday. Hold off if you dont mind ok?

Thanks for the code, let's keep the discussion going.

-Mat

@Incognito357
Copy link
Contributor Author

* What about the piano roll? This is what people seem to as for the most in term of auto-follow. Seems like
  we only handle the sequencer with this, right?

Yeah, this only affects the sequencer. I was thinking about the piano roll too, but I think it might be good for it to be separately configurable. It seemed like the piano roll would be more likely to need to be static while playing, since you wouldn't be able to really edit if it was following. Maybe having them as one setting would be perfectly fine, and having them separate would complicate things, and performing a pan or edit would disable following automatically.

* Also, assuming it affects both the sequencer + piano roll, I think a toggle in the toolbar would be better
  right? Seems like something you may way to change quite often? What do you think? (My _unofficial_
  rule for what belongs in the toolbar is stuff that applies to more than 1 of the UI component, sequencer,
  piano roll, etc.).

It does seem like something that you'd want to change on the fly, so a hotkey would be a good idea, and a toolbar icon could work as well. If we do two separate configs, the icon could possibly have a dropdown that would let you toggle one of the settings individually, though that starts to get a bit complex.

EDIT: Also, the code that follow should not be in the Rendering code, but rather in the Tick() code. Rendering code should stick to simply drawing.

I actually moved it to the tick function already, while I was trying to figure out the jittering issue, but I didn't commit since I was still trying to fix the jittering. Moving it actually makes the cursor jitter slightly, with (I think?) less jitter appearing from the rest of the rendering. I'm still not convinced though, because panning works 100% smoothly with the mouse, so it should be possible to get the continuous following to be smooth as well. It seems to happen independently of the song speed, so I think there might be something else happening.

EDIT2: Actually, ill take a crack a it on Friday. Hold off if you dont mind ok?

I haven't looked at the piano roll at all, but I assume it would probably have similar scrollX offset and cursor offset, so the same logic should be able to be applied as the sequencer. I'll hold off, but I'll keep investigating the jitter.

Thanks for the code, let's keep the discussion going.

-Mat

Sounds good, thanks for taking a look!

@BleuBleu
Copy link
Owner

BleuBleu commented Aug 26, 2020

The Jitter is likely due to the player running on a separate thread, so the current frame of the song can change at any moment.

Also, I would definitely prioritize the piano roll. Literally every request I got so far about this was for the piano roll. :)

EDIT: Maybe which one you want to follow (sequencer, piano or both) should be in the settings. And the master toggle on the toolbar?

-Mat

@Incognito357
Copy link
Contributor Author

Maybe which one you want to follow (sequencer, piano or both) should be in the settings. And the master toggle on the toolbar?

This seems like a good compromise. I'll take a look at adding a toolbar icon and modifying the config settings.
FWIW, I only have a linux and windows desktop to test on, unfortunately

- Following is now possible in both the sequencer and piano roll.
- Each window can be independently controlled (if only one is following, the
  other is automatically set to not follow), or both at once.
- The toolbar icon can toggle between the follow modes (none, jump, continuous),
  and right clicking toggles between windows following (seq, piano, both).
- Moved the follow code into the Tick() method instead of the Render() method.
- Temporary icons for the toggle window (there's 9 different states, they're
  all just words for now)
@Incognito357
Copy link
Contributor Author

Incognito357 commented Aug 26, 2020

Sorry I couldn't help myself, I added a toolbar icon and settings to control the piano roll, so of course I had to add the functionality to the piano roll as well while I was at it 😄

EDIT: I just tested on windows, and it works. The flickering happens a lot more on the piano roll, but I think moving the logic to the Tick() method helped, so the cursor is the thing that flickers and not so much the notes

@BleuBleu
Copy link
Owner

Ive been busy with regular work, ill try to take a look over the weekend.
If we want to include this, I don't want any jittering. It needs to be 100% flawless.

-Mat

@BleuBleu
Copy link
Owner

Also, side comment. I feel bad because you clearly put some work doing all these, but I will not use the icons you provided. They dont follow the established design language. Icons should be images, not text (only exception is PAL/NTSC since there was no established logo out there).

Also the alpha channel is not correct.

IconsPR

I will go with something like this:

IconsFixed

@Incognito357
Copy link
Contributor Author

That's fine, I didn't put any effort into the icons, it was more of a proof of concept with something to differentiate the states. I guess it shouldn't have been a pull request, or have just been a draft. I was thinking an animated icon would be able to convey the information more clearly than some kind of arrows and symbols could, but that didn't seem to fit the theme either

@BleuBleu
Copy link
Owner

Cool man, i merged it locally and I am playing with it. I think the feature will be awesome once its polished!

Hopefully i should have something good in a few hours, or tomorrow.

-Mat

@BleuBleu BleuBleu merged commit fac5559 into BleuBleu:2.2.0 Aug 30, 2020
@BleuBleu
Copy link
Owner

Hi!

So i checked in a version of the follow mode. I changes a couple things and polished it a bit let me know what you think.

The biggest change is that the "continuous" mode will not snap to the center of the screen. It will kick it when the cursor is 75% of the way and start scrolling from there. This will be consistent with how the new recording mode advances as well. Its not as visually impressive, but it is more practical to edit stuff in the center of the screen and have scrolling kick in as the seek bar reaches the right side of the screen. One day we might make this 75% configurable if there is demand for it, it is a constant for now.

I also eliminated the seek bar jitters, polished some rough edges like turning it off temporarily if you start panning, or adding notes, etc. I also made is zoom from the seek bar location in continuous mode, as it feels more natural.

The last type of jitter (which is almost impossible to eliminate, am im not gonna even try to fix) is dependent on of timing between the audio player threads and the refresh rate of the monitor. For example, playing a PAL song (50Hz), on a 60Hz monitor will never feel smooth.

Anyhow, thanks for the contribution.

-Mat

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.

None yet

2 participants