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

Webworker #595

Merged
merged 6 commits into from
Feb 21, 2018
Merged

Webworker #595

merged 6 commits into from
Feb 21, 2018

Conversation

thewtex
Copy link
Member

@thewtex thewtex commented Feb 19, 2018

This improves volume rendering initialization from 5.5 seconds to 2.1 seconds! ⚡

CC: @martinken @jourdain @agirault

This cuts initial volume load time in half.

Gradient computation occurs asynchronously in up to four web workers. These
worker are built into the final bundle inline with worker-loader.

VolumeMapper does not try to use the lightingTexture until after the gradients
have been computed.
@martinken
Copy link
Member

Can you change the test such that the new valid image is not needed? I would rather not add that valid image as it really isn't valid. It matches what I would expect the image to look like if the lighting/GO failed.

computedGradientsRenderTimeout = setTimeout(
model.openGLRenderWindow.modified,
20
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I get what this is doing in the timeout. Marking the renderWindow modified shouldn't change anything last I knew.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should make a comment here, but this triggers a render after the gradients are ready.

package.json Outdated
"seedrandom": "2.4.3",
"shelljs": "0.7.8",
"webworker-promise": "^0.4.1",
"worker-loader": "^1.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already provided inside kw-web-suite.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know -- I will see if we can build without it.


registerWebworker(async ({ array, min, max, numberOfBins }, emit) => {
const delta = max - min;
const histogram = new Float32Array(numberOfBins);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for float32 instead of uint32?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later, we normalize.

@thewtex
Copy link
Member Author

thewtex commented Feb 19, 2018

Can you change the test such that the new valid image is not needed? I would rather not add that valid image as it really isn't valid. It matches what I would expect the image to look like if the lighting/GO failed.

I will see if capturing the rendering snapshot after a timeout delay is possible and does the trick.

@thewtex
Copy link
Member Author

thewtex commented Feb 20, 2018

I will see if capturing the rendering snapshot after a timeout delay is possible and does the trick.

This does not work given the way that JavaScript executes code. The timeout is never reached.

@martinken Now, the volume mapper asynchronously re-renders with lighting effects enabled after the gradients have finished computing. If we really want to test this, I could try adding a .onLightingActivate or similarly named function to vtkVolumeMapper. This would be passed a callback that gets called when lighting is activated in vtkOpenGLVolumeMapper. In addition to re-rendering, this could grab a screen shot and perform the baseline comparison in the test. However, I am not sure how this should be exposed / propagated in vtkVolumeMapper and vtkOpenGLVolumeMapper. Thoughts?

@martinken
Copy link
Member

Thinking about this more generally. We need to know when "stuff" is done. Imagine if we did image flips in a worker. For FDA approval we would really need to be able to tell the doctor, yeh the image flips have all been done and what you are looking at is correct. Sort of a pending-work-is-all-done check. Not sure how JS handles this normally.

One thought, maybe when situations like this arise we instead request animation from the interactor and when done we cancel animation.

For tests we setup a callback on render that checks some state and if ready captures the test image and returns. @jourdain thoughts?

@jourdain
Copy link
Collaborator

We need to have some kind of progress handler which does not necessarily tell you how much progress has been made but if there is some work going on and when a given work is completed.

Those async handling are usually handle via promises and in lot of my web apps I have a wrapper around those promise to capture/monitor the busy state. We just need to add similar infrastructure for vtk.js.

This is already brought in by kw-web-suite.
@thewtex
Copy link
Member Author

thewtex commented Feb 20, 2018

@martinken @jourdain Yes, that sounds great. I am not sure how to implement it 😕 .

In the meantime, I looked into the callback approach, which could serve as a substitute until a promise-based render initialization infrastructure is in place. And, maybe this is used to inform the promise in the implementation? The test takes a bit longer to run, but it passes, and actually checks that lighting occurred :-). I will push the commit to the tip of this branch -- it could be omitted if desired.

This allows us to wait in the test to capture the render window after the gradients have been
calculated, and lighting has been enabled on the volume.
@@ -53,6 +54,7 @@ export function extend(publicAPI, model, initialValues = {}) {
'imageSampleDistance',
'maximumSamplesPerRay',
'autoAdjustSampleDistances',
'onLightingActivated',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use macro.event(publicAPI, model, 'lightingActivated') instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. 🍬 Done.

const onLightingActivated = model.renderable.getOnLightingActivated();
if (typeof onLightingActivated === 'function') {
onLightingActivated();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the event stuff, you will be able to call model.renderable.invokeLightingActivated().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@@ -91,16 +90,18 @@ test.onlyIfWebGL('Test Lighted Volume Rendering', (t) => {
renderer.getActiveCamera().zoom(1.4);
renderer.resetCameraClippingRange();
renderer.updateLightsGeometryToFollowCamera();
mapper.setOnLightingActivated(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will become mapper.onLightingActivated(() => {

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jourdain
Copy link
Collaborator

I'm guessing you validate the code with the event stuff?

@jourdain
Copy link
Collaborator

jourdain commented Feb 20, 2018

I'm also guessing that you haven't push yet...

@thewtex
Copy link
Member Author

thewtex commented Feb 20, 2018

Not yet, now I am debugging a timeout...

These babel bugs are encountered when used in the Karma tests:

  babel/babel#6956
  babel/babel#5085

Just explicitly return a Promise instead.
Use vtk.js event framework.

Break invocation recursion.

Avoid test timeout with `requestAnimation`.
@thewtex
Copy link
Member Author

thewtex commented Feb 21, 2018

Ran into some interesting bugs, including a bug in babel:

babel/babel#5085

All good now 🍰

@jourdain jourdain merged commit 3edf520 into Kitware:master Feb 21, 2018
@thewtex thewtex deleted the webworker branch February 21, 2018 13:30
@thewtex
Copy link
Member Author

thewtex commented Feb 21, 2018

Merci, @jourdain ! 🕺

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

Successfully merging this pull request may close these issues.

None yet

3 participants