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

Implement booleans #56

Closed
s-ol opened this issue Feb 6, 2022 · 5 comments · Fixed by #59
Closed

Implement booleans #56

s-ol opened this issue Feb 6, 2022 · 5 comments · Fixed by #59

Comments

@s-ol
Copy link

s-ol commented Feb 6, 2022

The boolean type tags are listed in the wiki but don't actually work:

Also, while in the OSC protocol booleans take no space in the message encoding, I think it would be beneficial to add true/false values into TypedMessage.args at the corresponding position. Otherwise the mapping from type to args is broken, which means the user has to essentially re-parse the type string just to find the corresponding value.

@adzialocha
Copy link
Owner

adzialocha commented Feb 16, 2022

Oh, thanks for the note @s-ol, think I've overlooked this one when merging #48.

@davidgranstrom, maybe you have an idea what is missing?

@davidgranstrom
Copy link
Contributor

@s-ol @adzialocha I think that adding corresponding values for the T,F (and I,N) types in order to keep symmetry between args and typetag makes sense.

It was a while since I wrote the PR so I would have to take a closer look to see what is actually needed. But I think it is mainly about adding the values in Message.unpack when looping over the typetag string and also accept values in the typeTag helper function. I don't think Message.pack would need to implement this since these types are not part of the binary OSC message data.

@adzialocha
Copy link
Owner

adzialocha commented Feb 16, 2022

Thank you @davidgranstrom !

I had a go on an implementation here, please have a look if you have time: #59

@davidgranstrom
Copy link
Contributor

@adzialocha That was fast :) Looks good to me! 👍

@adzialocha
Copy link
Owner

Super, lets release it then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants