Skip to content

fix: note pan min/max can be up to 128#9

Merged
demberto merged 5 commits intodemberto:masterfrom
zautumnz:fix/pan-min-max
Jul 22, 2022
Merged

fix: note pan min/max can be up to 128#9
demberto merged 5 commits intodemberto:masterfrom
zautumnz:fix/pan-min-max

Conversation

@zautumnz
Copy link
Copy Markdown
Contributor

@zautumnz zautumnz commented Jul 16, 2022

Note pan can be -128—128. Tested simply by adding one note on the piano roll and using the piano roll pan editor all the way up/down. On FL v 20.9.2 (build 2459), Producer/All Plugins edition, on Apple Silicon mac 12. Test FLP attached.
test-flp.zip

@zautumnz zautumnz changed the title fix: pan min/max can be up to 128 fix: note pan min/max can be up to 128 Jul 16, 2022
@demberto
Copy link
Copy Markdown
Owner

demberto commented Jul 16, 2022

Thanks for your contributions :)

I am actually doing an entire rewrite rn, none of those commits are pushed but, I am going for a more standard property based approach. _FLObject was long pending a refactor anyways.

I will merge this PR and make a patch release but in the near future everything is gonna get rewritten, so if you are making any changes to how the property handling or event parsing works, please don't.

Does FL Studio on Mac use UTF16 strings as well?

@tochibedford
Copy link
Copy Markdown

Is the rewrite going to cause issues with already written contributions @demberto

@zautumnz
Copy link
Copy Markdown
Contributor Author

zautumnz commented Jul 16, 2022

Good catch, I never really use Notebook2 or project info, so didn't notice that. Tested this commit with a Notebook2. Please let me know if that change is idiomatic; I'm kind of new to Python so not sure if it conforms to the style or not. If that's going to be overwritten by your rewrite I can revert that commit. I look forward to the rewrite!

@demberto
Copy link
Copy Markdown
Owner

demberto commented Jul 16, 2022

@zacanger TextEvents aren't encoded in UTF-8 in Mac, I just verified it with your test FLP. Notebook2's DataEvent might be using it on Mac. Also, the note pan min/max limits seem to be raised in 20.9 because my test FLP has similar notes with max/min pan.

Please let me know if that change is idiomatic

They will be idiomatic as well, since now I am better at Python myself. But most importantly, the property values will not be generated by all the parseprop and parse_event mess. They will be deserialized by the property getters from corresponding event data and serialized back by the setters. I have created helper prototypes to reduce code duplication created by this.

Along with this, after the events are all collected into a list by Parser, they won't be sent to the _FLObjects one by one as they are now. I will introduce event collectors in between which will gather events required for creating every model. After the collectors create their own sublists, I will pass them to constructors of the model classes. These constructors won't do any operations on them and store them in a private variable to be used by the getter/setters.

Or, I have been thinking of other way where the model classes (Channel, Track etc.) could be just simple dataclasses and serialization/deserialization would fall in the hands of collectors.

Also, I need to be very careful about validators in the future, probably discard their use completely. FLP is a very broken format at its core. The fact that they get invoked while parsing (its a bug) is the source of many problems.

@zautumnz
Copy link
Copy Markdown
Contributor Author

Thanks. I reverted the TextEvent change, left the one in Notebook2, but didn't touch DataEvent itself. I definitely look forward to your changes. I'm not surprised the FLP format is messy, since they've kept it mostly backwards-compatible for the last 24 years. It must be quite a chore keeping up with whatever Image Line are doing without good documentation on the format, so really, huge thanks for doing the work in this project :)

@demberto
Copy link
Copy Markdown
Owner

Hang on @zacanger, note pan is stored in a single byte internally and the range of a signed byte is -128 to 127. It can't be +128

Comment thread pyflp/pattern/note.py Outdated
@zautumnz zautumnz requested a review from demberto July 21, 2022 23:51
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

Merging #9 (99ec4f7) into master (132e4ca) will decrease coverage by 0.01%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
- Coverage   84.18%   84.17%   -0.02%     
==========================================
  Files          56       56              
  Lines        3857     3860       +3     
  Branches      641      641              
==========================================
+ Hits         3247     3249       +2     
- Misses        505      506       +1     
  Partials      105      105              
Impacted Files Coverage Δ
pyflp/plugin/effects/notebook2.py 69.09% <60.00%> (-0.14%) ⬇️
pyflp/pattern/note.py 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@demberto demberto merged commit c6b415e into demberto:master Jul 22, 2022
@zautumnz zautumnz deleted the fix/pan-min-max branch July 22, 2022 19:58
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.

4 participants