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

More about the the pan_l pan_r members #1242

Closed
oddtime opened this issue Apr 19, 2021 · 4 comments · Fixed by #1249
Closed

More about the the pan_l pan_r members #1242

oddtime opened this issue Apr 19, 2021 · 4 comments · Fixed by #1249

Comments

@oddtime
Copy link
Contributor

oddtime commented Apr 19, 2021

When we decided to keep pan_r and pan_l members in Note and Instrument class, we did it to maintain compatibility with the old song/drumkit files (see #665).

But actually we could maintain the compatibility deprecating those members (replaced by a unique pan member in [-1;1]) in Note and Instrument class, but still saving and importing the pan in the old way (using the pan law functions we have in Sampler class).
This avoids some conversions in the sampler, and simplifies the pan input/displaying in the GUI code reducing chance of errors.

Right?

@theGreatWhiteShark
Copy link
Contributor

With the design decision of early days to have two values and by maintaining backward compatibility we are bound to have some additional complexity somewhere in the code.

I agree that keeping both members will cause confusion and in the worst case even bugs. But having the classes and their file counterparts contain differing fields will cause confusion too.

With more XML knowledge up my sleeve than when I commented in #665 I would suggest to hide this complexity in the XSD file for validating the .h2pattern and drumkit.xml files and the method loading them.

@oddtime
Copy link
Contributor Author

oddtime commented Apr 24, 2021

So you would save single "pan" in xml files?

@theGreatWhiteShark
Copy link
Contributor

So you would save single "pan" in xml files?

Jupp. (Since fusing them to a single value is what I thought you were intending to do).

@oddtime
Copy link
Contributor Author

oddtime commented Apr 24, 2021

Yes

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 a pull request may close this issue.

2 participants