stijnfrishert / libLSDJ

Library for working with the LSDj save file format
MIT License
91 stars 6 forks source link

lsdsng-import shouldn't ignore FX/SPEED #61

Closed urbster1 closed 4 years ago

urbster1 commented 4 years ago

i thought instrument data was supposed to carry the whole chunk when importing? maybe something is weird but FX/SPEED was reset to 0 for inst 0E in this song. lsdsng (from LSDManager) and sav from lsdsng-import included. tested in v7.1.4 4thsriff.zip

urbster1 commented 4 years ago

FX/SPEED should show 1 for inst 0E (not zero)

stijnfrishert commented 4 years ago

Thanks, I'll take a look when I find the time!

stijnfrishert commented 4 years ago

Dependent upon #67

urbster1 commented 4 years ago

FX/SPEED has been renamed to CMD/RATE

stijnfrishert commented 4 years ago

Thanks! It's probably the same byte, but changes like these do propose an interesting question. What do we call these functions?

lsdj_instrument_set_fx_speed()? lsdj_instrument_set_cmd_rate()? Should we include both while one calls the other? People might look for one function over the other, depending on the LSDj version the library user is used to.

urbster1 commented 4 years ago

This one seems obvious, just call it cmd_rate since that's what it will be in the first stable version since the feature was added. But the P/L/V => PITCH change brings up a similar issue since past stable versions have had P/L/V and future versions will have PITCH (likewise PITCH vs FINETUNE in kits etc)

On Tue, Feb 11, 2020, 6:16 PM Stijn Frishert notifications@github.com wrote:

Thanks! It's probably the same byte, but changes like these do propose an interesting question. What do we call these functions?

lsdj_instrument_set_fx_speed()? lsdj_instrument_set_cmd_rate()? Should we include both while one calls the other? People might look for one function over the other, depending on the LSDj version the library user is used to.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stijnfrishert/liblsdj/issues/61?email_source=notifications&email_token=ABLMCWIUO4OEJL5Y3GZBDLDRCMWUNA5CNFSM4JXBGH7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELOPQBY#issuecomment-584906759, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLMCWLQTAMXQVGHG4XRYLTRCMWUNANCNFSM4JXBGH7A .

stijnfrishert commented 4 years ago

All right, that's good news for this issue, and then we'd have to think about P/L/V

urbster1 commented 4 years ago

right. and previously it was also called VIB TYPE. so we might have to think it through a bit

On Wed, Feb 12, 2020, 3:51 PM Stijn Frishert notifications@github.com wrote:

All right, that's good news for this issue, and then we'd have to think about P/L/V

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stijnfrishert/liblsdj/issues/61?email_source=notifications&email_token=ABLMCWMLCKC4FINOBZDZCWDRCRONVA5CNFSM4JXBGH7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELSKYSA#issuecomment-585411656, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABLMCWME6KBXZPRRZGTS2QDRCRONVANCNFSM4JXBGH7A .

stijnfrishert commented 4 years ago

So what setting is this exactly? Cause I didn't come across it in any of the version changes.

urbster1 commented 4 years ago

i got confused and started talking about something else. CMD/RATE (first known as FX/SPEED) controls the speed of C commands as well as P in noise channel (and P/L/V when pitch is set to TICK).

stijnfrishert commented 4 years ago

Hmmm, ok I'm not entirely sure where in Johan's upgrade code that's featured, so I'll look into that too.

stijnfrishert commented 4 years ago

Ok, this issue can actually be closed, the new v2 shouldn't ignore any settings at all anymore.

Opened #78 for post-v2 handling