Update vtk.js pointFeature

[jourdain]

No description provided.

[aashish24]

@manthey could you review this branch?

[manthey]

@jourdain This doesn't work as written, and I suspect that we are doing things in a more obtuse way than we should. For ease of trying this out, I've made two interactive tutorial links to allow adjusting the code. This link is the existing vtk.js pointFeature. This link is this branch (adjusted to work in the tutorial context).

[jourdain]

I noticed some issue here:

    var mapper = vtkMapper.newInstance({
      // Orientation
      // orient: false,

      // Color
      // useLookupTableScalarRange: true,  // FIXME <----
      // lookupTable,                      // FIXME <----
      colorByArrayName: 'color',
      colorMode: vtk.Rendering.Core.vtkMapper.ColorMode.MAP_SCALARS,
      scalarMode: vtk.Rendering.Core.vtkMapper.ScalarMode.USE_POINT_FIELD_DATA,
      scalarVisibility: true,

      // Opactity
      // => not available yet

      // Scaling
      scaling: true,
      scaleArray: 'radius',
      scaleMode: vtk.Rendering.Core.vtkGlyph3DMapper.ScaleModes.SCALE_BY_MAGNITUDE,
    });

But by fixing that, I get the following error:

map.js:1819 Error: Coordinates are not valid
    at Function.s.transformCoordinatesArray (VM64 geo.min.js:39)
    at Function.s.transformCoordinates (VM64 geo.min.js:39)
    at vtkjs_pointFeature._build (about:blank:156)
    at vtkjs_pointFeature._update (about:blank:192)
    at _renderFrame (VM64 geo.min.js:39)
    at V (VM64 geo.min.js:83)

[manthey]

I've pushed a change that fixes setting the radius. It still doesn't alter the color or opacity by point. I've marked some TODO spots.

Here is a tutorial editor link to this code, in case it is useful.

[jourdain]

Is there still something for me to look at?

[manthey]

@aashish24 This works. It has some mild eccentricities which I should document. For instance, when zoomed out, points become annular:

versus zoomed in:

Also, since in other renderers, points have strokes and/or fills, and here we only have fill values, I need to document that. I should also add one more test.

[manthey]

@jourdain I hacked around how the lookup table returns color information. I still have a color array, but the values of it don't seem to be used, and then I return a lookup table data array which has the packed rgba color values (which is where I end up supporting opacity).

I have two questions:

Is there a simple way to not need the extra color array?

Is there an example showing how to subclass something in vtkjs? It seems like it would be better for me to subclass LookupTable instead of abusing it the way I did.

[jourdain]

For class extension: https://kitware.github.io/vtk-js/docs/develop_class.html

What do you colorBy if you don't have the color array? The LookupTable is meant to convert a field into an actual color.

Since you seems to build the full RGBA for the full set of points, you should probably not use a lookup table and just use the raw data as an array in your pointset by setting "mapScalars=false".

[manthey]

@jourdain Thanks -- I've refactored this to be simpler. I was misunderstanding the vtk.js documentation.

[manthey]

@aashish24 This is ready for review.

[aashish24]

@aashish24 This is ready for review.

thanks. Do we still have the issue with "annular"? I believe that may be caused by clipping of spheres that are drawn by vtk.js.

[manthey]

thanks. Do we still have the issue with "annular"? I believe that may be caused by clipping of spheres that are drawn by vtk.js.

Yes. And, you are correct, it is the clipping. We can set the clip bounds to a larger range -- but I think that we should defer that to a future PR where we add a more general method to get the extents needed and set the bounds based on the actual extents (so we don't degrade z-buffer precision needlessly).

[aashish24]

Yes. And, you are correct, it is the clipping. We can set the clip bounds to a larger range -- but I think that we should defer that to a future PR where we add a more general method to get the extents needed and set the bounds based on the actual extents (so we don't degrade z-buffer precision needlessly).

sure sounds good. I guess this branch needs to be updated? Once that happens, I can approve it.

[manthey]

You can approve it while it updates -- the checks ensure that the merge to master will be valid.