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.
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
chore(core) new device props api #2100
chore(core) new device props api #2100
Changes from 2 commits
25c80e2
f5f8072
688f87f
c88dc08
49f332b
19ceb94
505a64a
14daa62
d9d23ab
4332836
c85cab8
fef139f
5c15612
51c451d
191661b
6a644a3
3dddd5d
2978f66
cb73d55
e3c192e
f253a5a
822f197
323c98d
d304728
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 is intended to be WebGPU only? It isn't used by WebGL
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.
What are these changes? Some editor auto correct?
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 calls the users onContextLost, if provided
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 see. Adding a one line bug fix in to a bigger API change is a bit risky, can slip through the review, and can get lost if API change is not approved...
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.
Happy to split this out. I didn't mean to put anything risky in here. The reason I included this is get clarity on the intended API. It's unclear to me what the intent is.
If users shouldn't be able to provide
onContextLost
by design, then the API types need to be adjusted a bit. If we can agree the user should be able to provideonContextLost
, then this is a bug and I'll pull that into another PR.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.
Now that you shine a light on it, I think we should completely drop support for
onContextLost
. Such callback code tends to be surprisingly messy to keep working across devices and it doesn't add value.We don't talk about contexts anymore, just devices. and we have the replacement in the WebGPU style promise API:
device.lost
App can register its onContextLost callback with
device.lost.then(onContextLost)
.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 agree.
device.lost
sounds better since it the same API for both devices, making this redundant.Is there a device equivalent for
onContextRestore
as well?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.
No, context restore is not supported by luma.gl v9.
The problems is that properly handling context restore in non-trivial apps is very tricky, since all resources are lost and have to be recreated, and the data that was used to create buffers and textures may not have been saved.
\Showing a message asking the user to just refresh the page is usually best way out.
WebGPU doesn't support restore.
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.
Should the onContextRestore code in the WebGL device be removed then?
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.
Yes. I would vote for that. It is not impossible for an app to register the callback even if luma.gl API doesn't support it, they will just need to get the context and add an event handler themselves.