-
Notifications
You must be signed in to change notification settings - Fork 9
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
update MRTextEntity to trigger update for innerText
manual changes
#561
Conversation
Signed-off-by: hanbollar <[email protected]>
Your Render PR Server URL is https://examples-mrjs-pr-561.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-co7infect0pc73aajg2g. |
Signed-off-by: hanbollar <[email protected]>
innerText
changes
innerText
changesinnerText
manually changes
innerText
manually changesinnerText
manual changes
Signed-off-by: hanbollar <[email protected]>
*/ | ||
async connected() { | ||
await super.connected(); | ||
_textWasManuallyUpdated() { |
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.
Why is this needed? Aren't we checking entity.textContent == entity.textObj.text
every frame?
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.
nope, we had removed that back when text updating was a big problem - right now we're only checking it when an overall eventUpdate
is triggered - the TextSystem
's update call is empty since it was too costly per frame: https://github.com/Volumetrics-io/mrjs/blob/main/src/core/componentSystems/TextSystem.js#L137
also in the incoming pr later where i rework some of the text system, i bring more of it into the main changes, but i still keep that update empty - didnt want to add all those system changes to this pr as even though it introduces readability and some fixes related to this - a lot of them arent needed until it gets merged
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.
hmmm I still think there might be a need for some stuff in the main update. particularly dynamic CSS updates, which we can't detect changes to via events.
This is good for now but dynamic CSS changes is a table stakes requirement for V1.
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.
s.g, dynamic CSS changes we can add to the main update in #546 since that's just the add of one function call - i'll keep this pr as is
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.
new issue #562
Linking
The actual fix is related to https://discord.com/channels/1137890872688660500/1219366027545149462/1225566696086044693
Most of the cleanup implementation in the pr is related to #546
Problem
Updating
innerText
directly only updates the DOM and not the scene itself until there is aneventUpdate
or direct update call to theSolution
initial thought - add it to the main update loopNOTE: i'm probably going to swap that in this pr once i finish writing this to a direct - if textContent was modified, do an event trigger update, because checking every loop is hella costlyupdating directly from the entity trigger √
Notes
once this pr is in, To see this live - check out the text example: https://examples.mrjs.io/examples/text.html
when locally testing (or before is in): https://localhost:8080/examples/text.html
Required to Merge
- [ ] VIDEO - if this pr changes something visually - post a video here of it in headset-MR and/or on desktop (depending on what it affects) for the reviewer to reference.- [ ] BREAKING CHANGE@lobau
@hanbollar