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

ofEvents::did_notify() #7773

Merged
merged 14 commits into from
Dec 12, 2023
Merged

Conversation

artificiel
Copy link
Contributor

as evoked in the forum this PR inserts an atomic bool in ofEvent that tracks if something happened with the Event, as well as an accessor that returns (and clears) the state since the last check (a bit like the isFrameNew() pattern). ofParameter is augmented with a pass-through method returning the state of it's internal Event.

it removes the need for a lot of boileplate listener callback functions, and simplifies the conditional handling of things. it also localizes processing within the sequential update() (or other) method, and ensures what is triggered by the parameter change happens on the main thread (i.e. another thread might trigger an event, which means the other thread will flip the bool (and potentially trigger listeners); the main thread will later read the bool and can "do something" with the new value). it also de-duplicates events: if an ofParameter changes 10 times between 2 update() passes, it will simply report "true" (instead of invoking 10 times the equivalent callback).

an example is provided.

CODE DEDUPLICATION: in the original code the ofEvent::notify() method is copied 4 times: 2 for the generic event, and 2 for the <void>-flavored events. These 2 copies are duplicated to support "no sender" vs "sender" versions. Calling the sender version with "nullptr" as the sender works in equivalence, so the PR thus does so and cuts in half the duplication. that commit was required in order to set the stage for the modified (state-tracking) version.

ATOMIC MEMORY ORDER: i'm acting like i understand enough about atomic memory_order to use it; i've opted for relaxed, apart from clearing the state when did_notify() returns true, which should be sequentially protected (in case multiple threads check for did_notifiy(); you don't want a second state to "simultaneously" read true while the false state has been reorganized in the pipeline. duplication of going true are OK so should be lock-free and mostly optimized away (which means the potential atomicity of the process may only have an impact in did_notify(), meaning cost-free if the feature is not used).

THE PR STRUCTURE: @dimitre mentioned here #7740 (comment) the notion of having bite-sized PR's, and it is indeed important to focus a PR on a precise topic, but it is sometimes difficult to have complete purity. in this case the first 2 commits are "preparing the ground" for the actual new proposal, followed by a clang-format, and the example. I consider this a typical package with well-named commits that can be inspected individually and chronologically, wrapping themselves in a single objective that boils down to a coherent single squashed commit into the OF tree.

@ofTheo
Copy link
Member

ofTheo commented Nov 26, 2023

@artificiel - thanks!
good to merge?

@artificiel
Copy link
Contributor Author

artificiel commented Nov 26, 2023

In terms of functionality seems complete. It works with standard events as well as ofParameter (including ofParameter<void> which has it’s quirks), and passes the current tests. It’s only an atomic bool who’s lifetime is tied to the ofEvent so it’s difficult to imagine some negative side effect, but the ofEvent system is a bit complex and maybe some edge cases exist.

Independently of what is specific to this feature, If we are considering of-git/nightly as « rolling release » (so presume a user might start working immediately with the feature expecting production-readiness) it would make sense to have a kind of status where recent « major » modifications are listed, and where some form of active confirmation (or reasonable timeout) removes them from the list prior to a packaged numbered release. I think that connects to my thoughts on the user-workflow of «git-latest» vs «git-bleeding» where git-bleeding would be a buffer where changes can spend a bit of time before being folded #7588 (comment)

in essence: quicker/riskier merges in -bleeding (so that PRs don’t accumulate and get stale; such a faster turnaround will also facilite smaller PRs that rely on each other) and frequent merges to latest (either after a period of inactivity/stability, or prior to a more involved change)

@c-mendoza
Copy link
Contributor

Can we rename it to didNotify()? That would be consistent with the current OF coding style.

@ofTheo
Copy link
Member

ofTheo commented Dec 12, 2023

hmm - looks like this is failing all the tests.
Love this PR btw! Going to make checking for changes so much easier.

@ofTheo ofTheo merged commit 7d10a73 into openframeworks:master Dec 12, 2023
9 checks passed
@artificiel artificiel deleted the events-did_notify branch December 12, 2023 22:40
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.

4 participants