Skip to content

Conversation

@jasp00
Copy link
Member

@jasp00 jasp00 commented Jun 26, 2016

Sometimes when loading projects like Greippi - Krem Kaakkuja (Second Flight Remix).mmpz, there are ID collisions and messages like JO-ID %d already in use by %s!. This may affect automation. To solve this, the range of IDs for new elements is different than for loaded ones. Another improvement is the optimization of assigning a random ID.

@jasp00 jasp00 merged commit 6470265 into LMMS:master Jul 2, 2016
@jasp00 jasp00 deleted the journal branch July 2, 2016 08:23
PhysSong added a commit to PhysSong/lmms that referenced this pull request May 27, 2018
Previously JournallingObject::changeID failed if _id is being used.
This commit adds handling of ID collision and partially reverts LMMS#2875.
As a side effect, it fixes automation TCO drag & drop bug introduced in LMMS#2875.
@DomClark
Copy link
Member

Apologies for commenting on this over two years later, but it looks to me like there might still be the potential here for ID collisions. Say a control with ID EO_ID_MSB | 0xf00 is automated, then it will be saved with ID 0xf00. Now, when the project is reloaded, it is possible for another control to be given ID EO_ID_MSB | 0xf00, which is now free, and if that were automated, an ID collision would occur when saving the file. The chances of this depend on the number of controls already automated, but EO_ID_MSB is 2^23 so they will tend to lie around 1 in a million. This sounds unlikely, but we have millions of users, so it's not unreasonable to think someone will encounter this. Is this an issue or am I reading the code wrong?

@PhysSong
Copy link
Member

That's why I'm trying to decouple automation from journaling. Journal ID should be unique in an instance of LMMS, while automation doesn't need to be.

@irrenhaus3 irrenhaus3 mentioned this pull request Jun 26, 2022
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
Split journal ID range between new and loaded elements
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.

3 participants