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

WIP: Implement Options and BufSize extensions #98

Draft
wants to merge 54 commits into
base: develop
Choose a base branch
from
Draft

Conversation

prokopyl
Copy link
Member

@prokopyl prokopyl commented Aug 9, 2021

This PR implements the Options spec, alongside the BufSize spec for a simple example of an use case.

This is still very much a WIP, as some APIs are not fully implemented or cleaned up, and it still needs some documentation.
In the meantime however, you can take a look at options/tests/optionable.rs for a simple example on how to use this. 🙂

Things still on my to-do list before I can mark this as ready for review:

  • Implement the "to atom value" side of OptionType
  • Finish implementing the OptionError conversions
  • Document all of the items

@prokopyl prokopyl added ✨ Feature New feature or request 💔 Breaking change Changes to the code that will lead to break one or more APIs labels Aug 9, 2021
@prokopyl prokopyl self-assigned this Aug 9, 2021
@prokopyl
Copy link
Member Author

prokopyl commented Aug 9, 2021

@Janonard @PieterPenninckx , if you wanna take a look on this, your feedback is welcome! 🙂

@prokopyl prokopyl added 🎁 New LV2 Spec Implements a new LV2 Specification and removed ✨ Feature New feature or request labels Aug 9, 2021
@PieterPenninckx
Copy link
Contributor

PieterPenninckx commented Aug 10, 2021

FYI: there's also this branch; it may serve as inspiration. Edit: this PR seems to be much more complete.

atom/src/lib.rs Outdated
@@ -171,6 +171,10 @@ where
) -> Option<Self::WriteHandle>;
}

pub trait BackAsSpace<'a>: Atom<'a, 'a> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm having trouble understanding what's going on.
Can you explain what this does and why it's used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I haven't documented anything yet ^^"

The "back as space" operation is something that takes an atom's read handle and turns it back into a space (i.e. back to a bare atom buffer).

This is necessary because, when reading options from the plugin, the LV2 options spec requests a pointer to an atom (alongside the regular size and type).
This is a completely different operation than what is usually done from the Atom ports: we're not writing to or reading from a buffer from the host, instead the host is basically requesting a buffer owned by the plugin with the value of the requested atom.

I've looked at implementations and usages of this, and the most usual case is options with scalars (mostly ints). In that case going from a read handle (&i32 in the case of Atom::Int) to a byte slice is trivial, however many atom types cannot be easily created from native types without writing them to a buffer first, so those cannot support the back_as_space operation.

That's why I had to make it a separate trait, it's because not every atom type supports that operation. 🙂

@PieterPenninckx
Copy link
Contributor

This is actually the first more-than-superficial review I'm doing for this crate. I've found some things that are unrelated to this PR, I'm fixing these in my fork.

@prokopyl
Copy link
Member Author

This is actually the first more-than-superficial review I'm doing for this crate. I've found some things that are unrelated to this PR, I'm fixing these in my fork.

Yes, I've found a few things too while working on this. I've put fixes and reworks on the atom-soundness branch, however it's very WIP, and I'm still experimenting on some things I'm not quite sure how to approach yet, that's why I haven't made a PR yet.

@PieterPenninckx
Copy link
Contributor

I've put fixes and reworks on the atom-soundness branch.

It looks like you've found much more than I have. I'll just document issues in my fork without an effort to actually fix things, so I can avoid duplicate work.

@PieterPenninckx
Copy link
Contributor

I had hoped to add support for LV2 in rsynth this week, which would give me enough background knowledge to review this PR, but alas... I'm afraid I won't be able to review this properly in the upcoming months. Looking at the BackAsSpace trait, I have the gut feeling that the "Space" mechanism for Atoms may need to be reviewed (simplified) in the light of this feature, but I may be wrong.

@prokopyl
Copy link
Member Author

I had hoped to add support for LV2 in rsynth this week, which would give me enough background knowledge to review this PR, but alas... I'm afraid I won't be able to review this properly in the upcoming months. Looking at the BackAsSpace trait, I have the gut feeling that the "Space" mechanism for Atoms may need to be reviewed (simplified) in the light of this feature, but I may be wrong.

@PieterPenninckx Indeed! It took quite a lot of time, but I made many changes, improvements, clarifications and simplifications in the Space APIs in #100 . 🙂

I will rebase this branch on it, and will remove the Draft tag on this PR when #100 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💔 Breaking change Changes to the code that will lead to break one or more APIs 🎁 New LV2 Spec Implements a new LV2 Specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants