-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add setImmediate implementation to the UI runtime #3970
Merged
Merged
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
229262b
Stashing work
kmagiera 651538c
Reset unrelated changes
kmagiera 37fdaf7
Breaking things, please don't use this commit, just trying differenty…
kmagiera 6a1dba4
Moar updates
kmagiera 2e41569
Even moar updates
kmagiera 4208b6d
A bit of cleanup
kmagiera 924a1b1
Fix Android builds
kmagiera f1a7860
Revert "Fix Android builds"
kmagiera 35fe5dc
Fix android builds for realz
kmagiera cf7e2b3
Merge branch 'main' into set-immediate
kmagiera 1cadbbe
Handle events with timestamp
kmagiera e024b5f
Minor fixes
kmagiera 81a7ef1
Fix cpplint
kmagiera 096c34a
Trying to fix tests (not yet passing)
kmagiera 0428f0c
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera ba92fbb
Fix some files after merging main
kmagiera e4db047
Lint/type fixes
kmagiera 6f43b36
''Fix'' tests
kmagiera b445d51
fix typoz
kmagiera 616db16
Moar test fixes
kmagiera 77b0151
Update comments
kmagiera eea48bf
Moar cleanup
kmagiera e3dc780
Further cleanups + update to node 16 because performance.now cannot b…
kmagiera 2c29b10
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera 914fa25
Merge remote-tracking branch 'origin/main' into set-immediate
kmagiera bfb4a93
Remove timestamp from mapper worklet agrument as it is not being used
kmagiera File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -132,24 +132,9 @@ void NativeProxy::installJSIBindings( | |||||
return static_cast<double>(output); | ||||||
}; | ||||||
|
||||||
auto requestRender = [this, getCurrentTime]( | ||||||
auto requestRender = [this]( | ||||||
std::function<void(double)> onRender, | ||||||
jsi::Runtime &rt) { | ||||||
// doNoUse -> NodesManager passes here a timestamp from choreographer which | ||||||
// is useless for us as we use diffrent timer to better handle events. The | ||||||
// lambda is translated to NodeManager.OnAnimationFrame and treated just | ||||||
// like reanimated 1 frame callbacks which make use of the timestamp. | ||||||
auto wrappedOnRender = [getCurrentTime, &rt, onRender](double doNotUse) { | ||||||
jsi::Object global = rt.global(); | ||||||
jsi::String frameTimestampName = | ||||||
jsi::String::createFromAscii(rt, "_frameTimestamp"); | ||||||
double frameTimestamp = getCurrentTime(); | ||||||
global.setProperty(rt, frameTimestampName, frameTimestamp); | ||||||
onRender(frameTimestamp); | ||||||
global.setProperty(rt, frameTimestampName, jsi::Value::undefined()); | ||||||
}; | ||||||
this->requestRender(std::move(wrappedOnRender)); | ||||||
}; | ||||||
jsi::Runtime &rt) { this->requestRender(onRender); }; | ||||||
|
||||||
#ifdef RCT_NEW_ARCH_ENABLED | ||||||
auto synchronouslyUpdateUIPropsFunction = | ||||||
|
@@ -306,14 +291,9 @@ void NativeProxy::installJSIBindings( | |||||
[weakModule, getCurrentTime]( | ||||||
std::string eventName, std::string eventAsString) { | ||||||
if (auto module = weakModule.lock()) { | ||||||
jsi::Object global = module->runtime->global(); | ||||||
jsi::String eventTimestampName = | ||||||
jsi::String::createFromAscii(*module->runtime, "_eventTimestamp"); | ||||||
global.setProperty( | ||||||
*module->runtime, eventTimestampName, getCurrentTime()); | ||||||
module->onEvent(eventName, eventAsString); | ||||||
global.setProperty( | ||||||
*module->runtime, eventTimestampName, jsi::Value::undefined()); | ||||||
// TODO: event time should be extracted from the system event object | ||||||
// instead of using the current time here. | ||||||
module->onEvent(getCurrentTime(), eventName, eventAsString); | ||||||
} | ||||||
}); | ||||||
#endif | ||||||
|
@@ -323,7 +303,7 @@ void NativeProxy::installJSIBindings( | |||||
std::shared_ptr<UIManager> uiManager = | ||||||
binding->getScheduler()->getUIManager(); | ||||||
module->setUIManager(uiManager); | ||||||
module->setNewestShadowNodesRegistry(newestShadowNodesRegistry_); | ||||||
module->setNewestShadowNodesRegistry(newezstShadowNodesRegistry_); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
newestShadowNodesRegistry_ = nullptr; | ||||||
#endif | ||||||
// removed temporary, new event listener mechanism need fix on the RN side | ||||||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to just move-construct
callbacks
fromframeCallbacks
here, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't my code so prefer not to make this change. Is this behavior defined that the original vector gets reset when moved? Typically move constructors does not specify behavior of the moved object as normally the assumption is that the moved object is never used again. Here we still want to use the vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine.
C++ standard says that the moved-from object is left in a "valid but unspecified state" (so it should be fine to use the old vector later), and the
std::vector
spec further specifies that "After the move,other
is guaranteed to be empty()." when move-constructing a vector, so it should clear it as well.In case you don't feel safe using a vector after moving from it, I would still suggest doing something like
since this is unambiguous in what it does, but still avoids copies/allocations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just like to avoid copying the callbacks here, since they can potentially capture a bunch of things - in this case it's a couple of
shared_ptr
s, which means unnecessary work on the atomic reference counters inside.