Improve the Wafer Map web worker class to support offscreen rendering #1929
Conversation
…e/Improve-the-web-worker-class-to-support-offscreen-rendering
Razvan1928
changed the title
Razvan1928
changed the title
…e/Improve-the-web-worker-class-to-support-offscreen-rendering
…e/Improve-the-web-worker-class-to-support-offscreen-rendering
packages/nimble-components/src/wafer-map/modules/zoom-handler.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/wafer-map-update-tracker.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/experimental/worker-renderer.ts
Outdated
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/modules/wafer-map-update-tracker.ts
Show resolved
Hide resolved
…e/Improve-the-web-worker-class-to-support-offscreen-rendering
…o-support-offscreen-rendering
| this.wafermap.experimentalDataManager.margin | ||
| ); | ||
|
|
||
| if (this.wafermap.diesTable !== undefined) { |
There was a problem hiding this comment.
I think all these checks for diesTable are undefined are incorrect because they result in the worker not receiving any message.
But the user could intentionally be trying to clear the current wafer that is being drawn. Now if you set diesTable to undefined nothing happens right (the wafer canvas stays the same even though the data was cleared)?
Maybe we are being too clever with the diesTable property and renderer. We should consider adding a boolean property like use-experimental-renderer and just be explicit for the user to opt-in.
I think there are two outcomes and they should not block this PR:
- setting and clearing the diesTable should behave well
- (up to y'all but I think it will help) we should make an explicit boolean configuration to switch rendering of the wafer map.
If that makes sense can you track it under Render Dies in the tracking issue? #2034
There was a problem hiding this comment.
So we have a flag on the wafer-map called isExperimentalRenderer() but it is checked before calling setupWafer(), inside update() from index.ts there is a
if (this.isExperimentalUpdate()) { this.currentTask = this.experimentalUpdate(); return; }
and the call stack is update() -> this.experimentalUpdate() -> this.workerRenderer.setupWafer();
so we can assume every time this.workerRenderer.setupWafer it is an experimental wafer
Regarding the undefined checking and the worker not receiveing messages, I replaced that check with
now the wafer is cleared when diesTable is undefined
There was a problem hiding this comment.
Reopening this question. @Razvan1928
- isExperimentalUpdate just checks that diesTable is undefined, are you sure that is sufficient? Did you do any manual verification?
- are there tests to verify the behavior change? Ie the wafer if drawn, then the data is changed to undefined, and the wafer is cleared?
If there are not tests there needs to be a follow-up PR
packages/nimble-components/src/wafer-map/modules/experimental/worker-renderer.ts
Show resolved
Hide resolved
packages/nimble-components/src/wafer-map/tests/wafer-map-matrix.stories.ts
Outdated
Show resolved
Hide resolved
Razvan1928
deleted the
users/rnazare/Improve-the-web-worker-class-to-support-offscreen-rendering
branch
Pull Request
🤨 Rationale
Wafer Map's canvas is the main component used to draw the actual dies on a wafer
The old wafer renders the canvas on the main thread, for the new one we want to render the canvas inside the web worker
👩💻 Implementation
The main changes were done in matrix-renderer.ts, worker-renderer.ts and index.ts
matrix-renderer.ts - holds the logic on how the canvas drawn, most of the changes are straightforward besides the typed array parsing algorithm from drawWafer(), but a comment explaining it can be found there
worker-renderer.ts - holds the logic on how worker is setup
index.ts - creates the worker and transfers the canvas control from main thread to the worker
less important changes were done in WaferMapTests.ts from the Blazor component and zoom-handler.ts
WaferMapTests.ts - after adding a new wafer map tag inside the wafermap template.ts the blazor tests were not able to distinguise between the 2 tags, I added IDs to each tag and changed the page.Locator to search for the ID and not for the tag name
zoom-handler.ts - zoom behavior was managed inside update() method from index.ts, as the constructor of ZoomHandler takes an wafer map as input we implemented the observable design pattern using Notifier from Fast Element, similar to how it is done for table nimble component
🧪 Testing
Added a few unit tests and a chromatic test
✅ Checklist