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

Medium-level API: Add PPQ functions #62

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Levitanus
Copy link
Contributor

midi_get_ppq_pos_start_of_measure
midi_get_ppq_pos_end_of_measure
midi_get_proj_qn_from_ppq_pos
midi_get_ppq_pos_from_proj_qn
midi_get_proj_time_from_ppq_pos
midi_get_ppq_pos_from_proj_time

every PPQ function is unsafe, because of take requirement

Copy link
Owner

@helgoboss helgoboss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Thanks!

main/medium/src/misc_newtypes.rs Outdated Show resolved Hide resolved
main/medium/src/misc_newtypes.rs Outdated Show resolved Hide resolved
main/medium/src/reaper.rs Outdated Show resolved Hide resolved
main/medium/src/reaper.rs Outdated Show resolved Hide resolved
@Levitanus
Copy link
Contributor Author

Now should be OK

@Levitanus
Copy link
Contributor Author

Levitanus commented Nov 13, 2022

I'm thinking on PositionInPpq representation. Basically, in take, PPQ is just normal u32. And, working with midi events, I usually need to convert it to the u32. It just returns from REAPER as f64.

It also blocks deriving of Ord, Eq, Hash. Changing underlying type to u32?

@helgoboss
Copy link
Owner

helgoboss commented Nov 20, 2022

I'm thinking on PositionInPpq representation. Basically, in take, PPQ is just normal u32. And, working with midi events, I usually need to convert it to the u32. It just returns from REAPER as f64.

It also blocks deriving of Ord, Eq, Hash. Changing underlying type to u32?

The question is whether REAPER supports and/or returns decimal PPQ values or is planning to do so in a potential future. If yes, we should definitely stay with f64. In such cases, I usually ask Justin by dropping him an email. Could you do that? If not or if that takes too long, I would prefer to take the safe route ... just use f64.

We can't derive Ord, Eq, Hash for that newtype with f64 but we can implement it. AFAIK, the most important reason why f64 doesn't implement those is that it can have special values such as NaN and so on. But we explicitly exclude that for the newtype, so we can provide an implementation that makes sense.

@Levitanus
Copy link
Contributor Author

Hmm, I never told to Justin)) If I try to ask some questions in cocos forum, it looks for me like an dead frost...

The question is whether REAPER supports and/or returns decimal PPQ values or is planning to do so in a potential future.

Reaper MIDI source is almost just a MIDI file of the standard midi specification. And PPQ is, generally, an summarized offset value. When MIDI take should be disconnected from the timeline, it happens not by multiplying values inside the pure midi data, but with modifying of the take tempo.

So, I think, the probability of fail is very low.


I've changed also `UsageScope` to `MainThreadOnly`

midi_get_ppq_pos_start_of_measure
midi_get_ppq_pos_end_of_measure
midi_get_proj_qn_from_ppq_pos
midi_get_ppq_pos_from_proj_qn
midi_get_proj_time_from_ppq_pos
midi_get_ppq_pos_from_proj_time

every PPQ function is unsafe, because of take requirement
Fixing naming convencions

Co-authored-by: helgoboss <[email protected]>
rebasing to master


Replaced all mut to ptr
@helgoboss
Copy link
Owner

Hmm, I never told to Justin)) If I try to ask some questions in cocos forum, it looks for me like an dead frost...

I asked him an he responded that internally it can be fractional but on the API surface it's essentially always an integer. So I guess we can go with an integer.

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.

2 participants