Skip to content

Ofx osc tweaks [needs #7620] - #7658

Merged
ofTheo merged 82 commits into
openframeworks:masterfrom
artificiel:ofxOsc-tweaks
Sep 29, 2023
Merged

Ofx osc tweaks [needs #7620]#7658
ofTheo merged 82 commits into
openframeworks:masterfrom
artificiel:ofxOsc-tweaks

Conversation

@artificiel

Copy link
Copy Markdown
Contributor

A collection of accumulated tweaks (tested in projects across macOS, iOS and linux), crossed with an audit of what was lying in GitHub issues.

I am submitting this as a "discussion PR" and am happy to chop it up as needed, but it seems more productive to present as a whole than a trickle of updates.

An example demonstrates most features: the constructors, the new friend std::optional, as well as method chains + overloaded-and-variadic add(). None of the changes involve "new design"; most of these changes are simply exposing latent C++ expressivity. No changes to current behaviour/performance. To be clear I am not advocating any of these "alternative" idioms as strictly better than the old school ones, but often they save lines of code, reduce redundancy, simplify usage, and enhance readability. All that translates to efficiency both in production or workshop contexts.

ofxOscSender:

  • (feature) new method isReady() (dynamic test) closes suggestion: ofxOsc .isSetup() function or equivalent #5186

  • (fix) changed return from void to bool for send() methods to report success of sending.

  • (feature) new flag to silence the error messages when invalid socket. Defaults to false (not silent) for backwards-compat. Added to Settings. With the bool flag on send(), this allows one to build a more adaptive harness around ofxOscSender without polluting the console, for instance in cases were it might be "not an error" for the receiver not to be available.

  • (feature) new ofxOscSender{string, int} to setup address:port; defaults to localhost:7970

  • (feature) new overloaded method send() accepts Messages and Bundles and ofParameters

ofxOscReceiver:

  • (feature) new ofxOscReceiver{int} to specify port at construction site; defaults to 7970

  • (feature) new method getMessage() that returns std::optional<ofxOscMessage> for "cleaner" while (const auto m = receiver.getMessage()) {…} syntax.

ofxOscMessage:

  • (feature) new ofxOscMessage{string} to specify address at construction site; no default

  • (feature) ofxOscMessage::add*arg methods returns &*this for method chaining

  • (feature) overloaded add() for unambiguous types, including variadic

ofxOscBundle:

ofxOscSync

oscpack:

  • (fix) C++17 warnings on sprintf() now considered unsafe -> requires snprintf() (the correctly defined required sizes were already lying around)

[help needed] Issues cleanup:

artificiel added 30 commits July 7, 2023 19:45
@artificiel

Copy link
Copy Markdown
Contributor Author

tests are failing because this is relying on #7620

@artificiel artificiel changed the title Ofx osc tweaks Ofx osc tweaks [needs #7620] Sep 18, 2023
@ofTheo

ofTheo commented Sep 28, 2023

Copy link
Copy Markdown
Member

@artificiel thanks for this!
Are there any regressions in this PR that you can think of, or should existing code continue to work as expected?

@artificiel

Copy link
Copy Markdown
Contributor Author

@ofTheo: the changes are mostly syntax additions (constructors, upgrade return types from void, some variadic methods), so the current behaviour (apart from adjusting the return type of ofxOscBundle) should not be modified -- it's one of the reasons I did not dig too deep in #5026 as it's touching the current processing. "anectodically" I've been using this branch for >12 months in 4-5 projects.

I realize there are many commits — it is OK to get them all squashed? for instance the oscpack changes are pretty specific and might warrant a more specific commit? perhaps also the return type changes? I don't know what kind of control GitHub provides on the granularity, and maybe it does not matter so much. As I mentioned above they're lumped in as single PR as they cover a coherent "topic" and can be reviewed as such.

@ofTheo

ofTheo commented Sep 28, 2023

Copy link
Copy Markdown
Member

Thanks @artificiel
Would be great if the oscOptionalExample could have all the standard ofApp functions and decoration.

But apart from that, I think this all looks good to merge as a mega commit :)
I think it's basically all or nothing with the squash options.

@artificiel

Copy link
Copy Markdown
Contributor Author

@ofTheo to be sure, what do you mean by "all the standard ofApp functions and decoration"?

I took the habit of commenting out the unused parts of the interface as it makes it clearer for beginners to focus on what actually does something in a source code file. having the unused ofBaseApp methods greyed-out (but ready to be activated without fishing them "out of nowhere" (again, beginner p.o.v)) is a good compromise as far as implicit documentation goes, and a good way to introduce the concept of interface — "if you want to handle the mouse events... blahblah".

(on the topic in my own workshop files in a similar vein I generally use the single-file main.cpp approach as not having to navigate between windows/files when explaining/demonstrating makes things much more fluid, and for those coming from python or javascript it postpones a bit the requirement of learning about separating interface from implementation).

@ofTheo

ofTheo commented Sep 28, 2023

Copy link
Copy Markdown
Member

@artificiel In general we keep all the functions that are in emptyExample.

While I get your point, we've found it's helpful to have the empty functions so beginners can quickly add functionality to an example ( often times people modify the examples as a step towards making something new ).

For example: If you want to add a key press to trigger something, a beginner then has to uncomment it in the .h and then add it in the .ccp which is extra friction.

Anyway, good to match the general practice if possible. :)

@ofTheo

ofTheo commented Sep 28, 2023

Copy link
Copy Markdown
Member

Also I would say changing the style/format of all the examples is definitely a discussion that can be had.
There are probably better approaches.

It's just better to do that after a discussion and then PR all in one go instead of each example being in a completely different approach.

@ofTheo
ofTheo merged commit 99b5995 into openframeworks:master Sep 29, 2023
@ofTheo

ofTheo commented Sep 29, 2023

Copy link
Copy Markdown
Member

woohoooo! 🎉

@artificiel
artificiel deleted the ofxOsc-tweaks branch September 29, 2023 03:32
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.

oscParameters Receiver + Sender not working with ofParameter<void> suggestion: ofxOsc .isSetup() function or equivalent

4 participants