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

WebGL renderer does not support tile-drawn / tile-drawing #2483

Open
thec0keman opened this issue Feb 27, 2024 · 10 comments
Open

WebGL renderer does not support tile-drawn / tile-drawing #2483

thec0keman opened this issue Feb 27, 2024 · 10 comments

Comments

@thec0keman
Copy link

It appears from the source code that the WebGL drawer does not support the tile-drawn / tile-drawing events:
https://github.com/openseadragon/openseadragon/blob/master/src/webgldrawer.js#L102

The tile-ready event seems to be the equivalent.

Two things:

  1. It would be helpful to update the documentation for these events to indicate this distinction. (Given the scope of changes with the WebGL renderer it might even be helpful to have a section dedicated to upgrading to it?)

  2. Should rejectEventHandler be a bit noisier about rejected callbacks? It looks like from the code it should be logging something, but we aren't seeing any errors logged for this.

@pearcetm
Copy link
Contributor

The reason that these events aren't supported in the new drawer is that the webgl drawing pipeline doesn't allow programmatically interacting with the pixel data during drawing like the canvas drawer does (which is what those events allow). Exactly what the preferred way to do this will be will depend on the in-progress cache overhaul PR (#2407). Until this is figured out, I don't think we know what a new section of the documentation site should look like. Your input would be very welcome though!

The JSDocs have already been updated as part of the webgl PR to include the info about tile-drawn and tile-drawing not being supported by the webgl drawer. I would guess the online docs probably won't reflect this until the next release, however... is that right, @iangilman?

In addition to these changes to the docs, there's also a new mechanism that logs a warning if you try to listen for one of these non-supported events - see #2310 (comment). The hope is that this will help developers notice this change ASAP if they happen to be using tile-drawing or tile-drawn.

@thec0keman
Copy link
Author

Thanks for the explanation @pearcetm ! Is there any special setup required to see the exceptions when listening to a non-supported event? I saw that code and was just surprised I hadn't seen any warnings while debugging tile-drawn callbacks not firing.

@pearcetm
Copy link
Contributor

pearcetm commented Feb 28, 2024

Is there any special setup required to see the exceptions when listening to a non-supported event?

There shouldn't be... in the drawer comparison demo (at http://localhost:8000/test/demo/drawercomparison.html, if you've run grunt dev) I get the following when adding handlers to the canvas drawer (viewer1) and the webgl drawer (viewer2) in the console:
image

How are you adding handlers for this event? What are you seeing in the console?

@iangilman
Copy link
Member

I would guess the online docs probably won't reflect this until the next release, however... is that right, @iangilman?

Correct!

@Aiosa
Copy link
Contributor

Aiosa commented Mar 4, 2024

We will have to documment a lot of stuff. My current idea is that instead of tile-drawn and related canvas drawer oriented events, a generic 'invalidate tile' pipeline will be added. This will allow you to

So you don't react to what a drawer is doing, but rather request an update event and do your magic. The former approch does not work since

  • generic drawer can have many ways of rendering, not all rendering pipelines let you add aditional steps at will (WebGL, WebGPU...)
  • the new async cache (as it is being designed) will NOT make renderer wait so even if the event happens, your handlers will have to use the cache system (no more context2D prop hacks) and will have to work with async calls, so the logics will happen after the render animation anyway, not during

I was thinking whether we really need the cache to be async, we could just drop support going to image type (e.g. we could create canvas from image but not vice versa), which is I think the only case that needs async execution. But such design (although much easier to program and 'friendlier' to the old OSD ways) is also very limiting - any type conversion that requires callback or promises would not be supported. Optionally, we could just allow users to switch between async and sync pipelines, but that would IMHO lead to a lot of confusion later on.

@iangilman
Copy link
Member

@Aiosa Agreed! Thank you for sharing this context.

@ghost
Copy link

ghost commented Mar 7, 2024

hey @iangilman , could you please assign this issue to me so that i can work on it.

@iangilman
Copy link
Member

@hem4nn We're talking about a lot of documentation here, and I think @pearcetm is in the best position to understand what needs to be written. That said, perhaps you could help... Do you have a good sense of what we're talking about and what info needs to be documented?

@ghost
Copy link

ghost commented Mar 7, 2024

yeah thats a lot, anyway i'll try to align them up and make up my first contribution, If i got any doubts sure i'll let you know about it @iangilman.

@pearcetm
Copy link
Contributor

pearcetm commented Mar 7, 2024

Much of the current issue is that the online docs don't yet reflect what has already been documented in the source code. The online docs will change to reflect this at the time of the next release.

The majority of what still needs to be documented will depend on #2407, since that will determine what the new APIs are that can serve as a replacement for the use-cases where tile-drawing/tile-drawn are currently used.

So, I don't think there's actually much to do right at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants