MarcBoule / ImpromptuModular

Virtual Eurorack Modules for VCV Rack
Other
93 stars 10 forks source link

Clocked syncing bug when duplicating a master Clocked #63

Closed fractalgee closed 1 year ago

fractalgee commented 1 year ago

1) create master Clocked in patch (edit: set it to 24ppqn mode. my master clock always is in this mode that is why I didn't even think of that) 2) duplicate that master Clocked 3) set initially added Clocked to master 4) connect duplicate to master 5) create a brand new Clocked from module browser 6) connect this new Clocked to master 7) hit Run on master Clocked. Second (duplicate) seems to think it is master too, even though not set so. 8) see 3rd added from module browser working as intended.

Patch attached shows that, just remove the .zip suffix, added only to make github happy

Clocked Sync bug.vcv.zip

MacOS 13.2 Rack Pro 2.2.3 Impromptu 2.1.1

MarcBoule commented 1 year ago

You have to do a reset first, once all the clocks are patched (or turn on some of the "on reset" options in the menus), because in the steps you describe, it's working as per expected behavior given that run just pauses the clocks and so without any resets it's normal they are not in sync.

fractalgee commented 1 year ago

The first was not running and sends a reset pulse on start to the duplicate which stays not running, as on start send rest is on on first. The newly added one indeed works as expected. I can reset the second as often as I want here, the duplicate 2nd one never wakes up or starts to run, whatever the state may be.

Did you check my patch? Only your modules in it and an audio module. It shows it clearly and it 100% reproducible here. Or does that patch also not show it there? Then we maybe have a mac versus win/lin (not sure what you dev on) issue here methinks.

Petervos2018 commented 1 year ago

Hi Fractal, checked your patch and indeed it is FFed up. The way of cloning, making different clocks masters an auto connecting the slaves makes a mess of the clocked trio in your patch. So it's a bug 😃

MarcBoule commented 1 year ago

Thanks for supplying the patch, but with the detailed steps to reproduce, I didn't need to try it. I simply performed the steps outlined above, and when I toggle Run on the first clock, the two others' Run buttons are following it perfectly. As I said above, for the clock signals to be in sync, it needs a reset first, but after that, they all seem to be in sync.

Also, just to make sure the terminology is clear, the "Master" designation in Clocked is only for auto-patching purposes, it has nothing to do with any other internal shared states or signals. It just saves you from connecting the cables manually.

I'll try the patch later, but if you want to add any missing details to the 8 steps above, please feel free, as when I do those 8 steps I see nothing weird.

fractalgee commented 1 year ago

The patch here shows both the first and dupe clock having the clock sync light on, so dupe clock seems to think it is master as well even though not set to it. I did exactly those steps to create that patch so not sure why things would be different for you.....

On Sun, 12 Feb 2023 at 19:13, Marc Boulé @.***> wrote:

Thanks for supplying the patch, but with the detailed steps to reproduce, I didn't need to try it. I simply performed the steps outlined above, and when I toggle Run on the first clock, the two others' Run buttons are following it perfectly. As I said above, for the clock signals to be in sync, it needs a reset first, but after that, they all seem to be in sync.

Also, just to make sure the terminology is clear, the "Master" designation in Clocked is only for auto-patching purposes, it has nothing to do with any other internal shared stated or signals. It just saves you from connecting the cables manually.

I'll try the patch later, but if you want to add any missing details to the 8 steps above, please feel free, as when I do those 8 steps I see nothing weird.

— Reply to this email directly, view it on GitHub https://github.com/MarcBoule/ImpromptuModular/issues/63#issuecomment-1427097138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7FMADYMBZRLDT3ESZG74LWXER4BANCNFSM6AAAAAAUZNKRYE . You are receiving this because you authored the thread.Message ID: @.***>

MarcBoule commented 1 year ago

I did exactly those steps to create that patch

Are you sure about this, because in your patch you have the first two clocks in P24 mode, but none of the 8 steps mentions changing the mode.

In any case, in order to get good behavior, you have to set the 2nd clock to CV mode (the 3rd clock is already in CV mode, so it's ok). Then toggle the Run button on the 2nd and 3rd clocks to put them in the same state as the first one. Hit reset, and then you're all set, toggling the first clock will toggle the two other ones and everything should be good.

fractalgee commented 1 year ago

uhm, ok, not intuitive and I scratched head for ages, as third clock is fine slaved to first which is in 24ppqn mode, just the cloned one isn't. Sure that this is how things should work? Should the cloned slave not immediately go to CV mode if slaved (auto-patched) instead? As that is only thing I didn't check when I messed with this. so cloning a master in 24ppqn mode is messed up and one that was not cloned is fine is very difficult to work out. Should need no reset just like the newly added 3rd one methinks, but that does get it to work indeed if I set clone to CV mode first

On Sun, 12 Feb 2023 at 20:17, Marc Boulé @.***> wrote:

I did exactly those steps to create that patch

Are you sure about this, because in your patch you have the first two clocks in P24 mode, but none of the 8 steps mentions changing the mode.

In any case, in order to get good behavior, you have to set the 2nd clock to CV mode (the 3rd clock is already in CV mode, so it's ok). Then toggle the Run button on the 2nd and 3rd clocks to put them in the same state as the first one. Hit reset, and then you're all set, toggling the first clock will toggle the two other ones and everything should be good.

— Reply to this email directly, view it on GitHub https://github.com/MarcBoule/ImpromptuModular/issues/63#issuecomment-1427110906, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7FMAC3O23QEMLAZJBCFKLWXEZMPANCNFSM6AAAAAAUZNKRYE . You are receiving this because you authored the thread.Message ID: @.***>

MarcBoule commented 1 year ago

Cloning in Rack literally duplicates the state, so if the 1st one is in P24, then the cloned one will be also. But if the cloned one then gets autopatched, then we must set it back to CV mode, but I see your point that it could do this automatically, since when auto patching, we assume we are going to be a slave (which inherently does not use the P modes).

fractalgee commented 1 year ago

That aligns with how I believe it should be, as clone makes no sense to stay in 24ppqn mode when auto-patching clone to 24ppqn master. So that would be a perfect fix to handle this. Sorry it never occurred to me that that 24ppqn setting on master may bork things up

On Sun, 12 Feb 2023 at 20:36, Marc Boulé @.***> wrote:

Cloning in Rack literally duplicates the state, so if the 1st one is in P24, then the cloned one will be also. But if the cloned one then gets autopatched, then we must set it back to CV mode, but I see your point that it could do this automatically, since when auto patching, we assume we are going to be a slave (which inherently does not use the P modes).

— Reply to this email directly, view it on GitHub https://github.com/MarcBoule/ImpromptuModular/issues/63#issuecomment-1427114522, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7FMAC2636JZZNQCIHFNCTWXE3T7ANCNFSM6AAAAAAUZNKRYE . You are receiving this because you authored the thread.Message ID: @.***>

MarcBoule commented 1 year ago

No problem, at least we got to the bottom of it! :-)

fractalgee commented 1 year ago

Rabbit hole goes slightly deeper, if one clones a 24ppqn master and then manually connect it to the master then should it not also automatically assume it should sync to master as it obviously is a slave, or am I overthinking this?

On Sun, 12 Feb 2023 at 20:40, Georg Carlson @.***> wrote:

That aligns with how I believe it should be, as clone makes no sense to stay in 24ppqn mode when auto-patching clone to 24ppqn master. So that would be a perfect fix to handle this. Sorry it never occurred to me that that 24ppqn setting on master may bork things up

On Sun, 12 Feb 2023 at 20:36, Marc Boulé @.***> wrote:

Cloning in Rack literally duplicates the state, so if the 1st one is in P24, then the cloned one will be also. But if the cloned one then gets autopatched, then we must set it back to CV mode, but I see your point that it could do this automatically, since when auto patching, we assume we are going to be a slave (which inherently does not use the P modes).

— Reply to this email directly, view it on GitHub https://github.com/MarcBoule/ImpromptuModular/issues/63#issuecomment-1427114522, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC7FMAC2636JZZNQCIHFNCTWXE3T7ANCNFSM6AAAAAAUZNKRYE . You are receiving this because you authored the thread.Message ID: @.***>

MarcBoule commented 1 year ago

This tweak will be in the next release (which will likely take some time, so please keep it in mind until then). Thanks!

MarcBoule commented 1 year ago

Rabbit hole goes slightly deeper, if one clones a 24ppqn master and then manually connect it to the master then should it not also automatically assume it should sync to master as it obviously is a slave, or am I overthinking this?

Although I see your point with a manual connection, as some point it becomes a bit excessive to try to predict all these things and make setting change automatically according to what the user is doing. I was willing to do it for autopatch, as there was another internal setting that was in fact being changed, but for manual patching I'm not sure I want to open up that can of worms.

fractalgee commented 1 year ago

autopatch is fine, just thinking a bit off the beaten path I guess, your call, your code, all good. Thanks for getting the one for autopatched slave clone done, will be very much appreciated, my further lamentations notwithstanding, I was just thinking if cloned slave gets bpm connected for a clocked it knows is master alreay then it could just switch to CV mode, as that would have made sense to me, alas, understand you don't want to persue that route

fractalgee commented 1 year ago

one more possible odd one here: If I dupe a master in 24ppqn mode the new one automatically becomes master. That is non-intuitive. You decide ...

MarcBoule commented 1 year ago

Yeah, I totally agree, and I would like to fix that, but I don't know how. In the module itself, the code that gets called to load a state when a module is duplicated is the exact same as the code that gets called when a patch is reloaded (and when we do a paste in the right click menu, etc). The method is called data_from_json(...) and all Rack modules must put their code in that, to reload the module's state, but in that method we don't know why we're being reloaded, and we can't tell the difference betweem duplication and patch loading.

So if I make it not reload the "master-ness" status, then it will work correctly on duplication, but not when we reload the patch. It's a catch-22!

fractalgee commented 1 year ago

ah, one of those, nvm then. it is at least mostly fixed with that fix of yours. All good...