这是indexloc提供的服务,不要输入任何密码
Skip to content

feat: add an OscOutput operator and its example #404

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

Merged
merged 6 commits into from
Mar 28, 2024

Conversation

ylmrx
Copy link
Contributor

@ylmrx ylmrx commented Mar 26, 2024

This is a new operator, counterpart to OscInput
It only supports strings and float numbers, it tries to fail gracefully on bad location and failure to connect.
Error and state check is grunty, please advise if I'm doing it too wrong.

Copy link
Collaborator

@pixtur pixtur left a comment

Choose a reason for hiding this comment

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

That's an excellent PR! Most of my comments are nitpicky about code-style and conventions. But there are three things that should be done:

  1. The Op should implement the Dispose-Pattern and probably disconnect when disposing. (If not, it might happen that the op keeps an connection open).
  2. Another problem might be that too many open connections will lead to performance issues. I'm not sure if this is true but OscInput uses a shared OscConnectionManager to pool and reusing connections. Although this optimization might be quite involved. I think we can live without that here.
  3. Please implement IStatusProvider. This will show a warning icon whenever the op has a problem (like a missing connection or an invalid IP address). Check [AbletonLiveSync] for an example.

@ylmrx
Copy link
Contributor Author

ylmrx commented Mar 28, 2024

(disregard latest commit, involving very experimental/wip/poc regarding text support for OscInput, i wanna keep this change focused on OscOutput), switching it to draft, as i might be noisy in between)

@ylmrx ylmrx marked this pull request as draft March 28, 2024 15:25
@ylmrx ylmrx force-pushed the feat/osc_output_operator branch from 4f0e76c to 7ec3579 Compare March 28, 2024 15:32
@ylmrx
Copy link
Contributor Author

ylmrx commented Mar 28, 2024

That's an excellent PR! Most of my comments are nitpicky about code-style and conventions.
But there are three things that should be done:

Thanks ! :)

The Op should implement the Dispose-Pattern and probably disconnect when disposing.
(If not, it might happen that the op keeps an connection open).

done.

Another problem might be that too many open connections will lead to performance
issues. I'm not sure if this is true but OscInput uses a shared OscConnectionManager to
pool and reusing connections. Although this optimization might be quite involved.
I think we can live without that here.

The conManager is built with a server purpose in mind, and won't be much use for us here.

I'm not sure a threaded model would help us in this context, as there's no error check on the OscRug.foobar.Send() (we're UDP), and a given operator should address its whole traffic (all distinct IPs/port/location)

We can check a later stage, for a threaded model, if we get performance issue, but i think we're fine here.

On another, perf-related note, the operator gives a tiny glitch once in while, but seems to be related to the refresh rate of Tooll (and phase with touchosc frame rate (where you can't disable vsync) : it completely disappear when I disable vsync (in tooll) and it stays well under 1ms frame-time).

This is a general issue with OSC client (TouchOSC does that too (this is why many standalone sequencer/drumbox were attempted and failed (15ms is barely noticeable for eye, but sound like a clumsy drummer for a sequencer)

Please implement IStatusProvider. This will show a warning icon whenever the op
has a problem (like a missing connection or an invalid IP address). Check [AbletonLiveSync]
for an example.

Done!

(edit: quotes readability, bis..)

@ylmrx ylmrx marked this pull request as ready for review March 28, 2024 19:18
@pixtur pixtur merged commit b0e8e71 into tixl3d:dev Mar 28, 2024
@pixtur
Copy link
Collaborator

pixtur commented Mar 28, 2024

Awesome!

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