saalfeldlab / paintera

GNU General Public License v2.0
99 stars 17 forks source link

Add 3d mesh visualization for thresholded sources/masks #377

Closed hanslovsky closed 4 years ago

hanslovsky commented 4 years ago

Currently (and probably also in the future), meshes are toggled off by default for thresholded sources. The meshes can be toggled on with the checkbox in the "Meshes" tab of a thresholded source in the preference pane.

Have only tried with CREMI raw data so far, and the highest resolution scale level I could view (on my laptop) is scale level 4.

hanslovsky commented 4 years ago

cc @axtimwalde if you'd like to try it. Note: This is still heavily WIP

hanslovsky commented 4 years ago

This is ready for review now.

I slightly changed the MeshSettings class to allow for different default values. While for neurons, we do want to show the 3D representation by default, this may not be the case for the thresholded source state.

igorpisarev commented 4 years ago

Unless it's urgent, I humbly ask to wait with merging this PR into master until #267 is merged. The adaptive resolution branch has ~100 changed files and it's extremely difficult to resolve merge conflicts there. It would likely be much easier to merge this branch into the new master once adaptive-mesh-resolution is merged rather than the other way around.

igorpisarev commented 4 years ago

I'm currently testing this branch to visualize some predictions, and I found a small bug: if the min/max threshold values are changed and the mesh is re-generated, in addition to that another mesh at the lowest resolution is added to the scene. This dangling mesh goes away if the user forces to refresh the meshes. It's especially noticeable when a higher scale level is used in the mesh generation settings.

hanslovsky commented 4 years ago

I'm currently testing this branch to visualize some predictions, and I found a small bug: if the min/max threshold values are changed and the mesh is re-generated, in addition to that another mesh at the lowest resolution is added to the scene. This dangling mesh goes away if the user forces to refresh the meshes. It's especially noticeable when a higher scale level is used in the mesh generation settings.

Thanks for testing, I will have a look!

Unless it's urgent, I humbly ask to wait with merging this PR into master until #267 is merged. The adaptive resolution branch has ~100 changed files and it's extremely difficult to resolve merge conflicts there. It would likely be much easier to merge this branch into the new master once adaptive-mesh-resolution is merged rather than the other way around.

This PR can wait until the adaptive-mesh-resolution branch is merged into master.

hanslovsky commented 4 years ago

@igorpisarev The issue that you observe may be a timing issue. I was unable to reproduce on my laptop so far but I will try on my workstation (which probably has a more similar configuration to your workstation), as well. I do have a vague idea what might cause it and will have a look.

igorpisarev commented 4 years ago

The dangling mesh I'm observing is generated at the lowest resolution scale level regardless of the mesh settings, and is reproducible every time (I only tried it on my workstation so far). But if the bug is in MeshGenerator or MeshGeneratorJobManager, it will likely be automatically resolved after merging with #267.

hanslovsky commented 4 years ago

Does it ever re-occur after the first time refreshing the meshes?

igorpisarev commented 4 years ago

Yes, if I hit refresh to remove the buggy mesh and then change the threshold values, it occurs again.

hanslovsky commented 4 years ago

This is currently not working but f062c09 saved WIP after merging current master with adaptive mesh resolution into this branch. Follow the discussion on the gitter for details on why and possible solutions.

hanslovsky commented 4 years ago

I am currently updating to the new rendering mode with adaptive mesh resolution in a separate branch and will eventually squash into a single commit and fast-forward merge into this branch once ready.

igorpisarev commented 4 years ago

@hanslovsky It could also be helpful to run with -ea. There is quite a bit of assertions in the new mesh generation code, so if something is not working this may help to debug.

hanslovsky commented 4 years ago

Thanks @igorpisarev

hanslovsky commented 4 years ago

I fixed the paint/voxel count issue of the canvas in bf9353b. I will test some more and then merge into master.

I think that the progress bar issues should be addressed in a separate PR because they seem unrelated.

hanslovsky commented 4 years ago

This currently throws an exception when the color seed is changed through the C key:

Exception in thread "JavaFX Application Thread" java.lang.IndexOutOfBoundsException: Index: 6, Size: 6
    at java.util.ArrayList.rangeCheck(ArrayList.java:657)
    at java.util.ArrayList.get(ArrayList.java:433)
    at org.janelia.saalfeldlab.fx.ObservableWithListenersList.stateChanged(ObservableWithListenersList.kt:51)
    at org.janelia.saalfeldlab.paintera.stream.AbstractHighlightingARGBStream.clearCache(AbstractHighlightingARGBStream.java:259)
    at org.janelia.saalfeldlab.paintera.stream.HighlightingStreamConverterConfigNode$node$4$1.changed(HighlightingStreamConverterConfigNode.kt:128)
    at org.janelia.saalfeldlab.paintera.stream.HighlightingStreamConverterConfigNode$node$4$1.changed(HighlightingStreamConverterConfigNode.kt:39)
    at com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:361)
    at com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:81)
    at javafx.beans.property.LongPropertyBase.fireValueChangedEvent(LongPropertyBase.java:106)
    at javafx.beans.property.LongPropertyBase.markInvalid(LongPropertyBase.java:113)
    at javafx.beans.property.LongPropertyBase.set(LongPropertyBase.java:147)
    at javafx.beans.property.LongProperty.setValue(LongProperty.java:72)
    at org.janelia.saalfeldlab.paintera.stream.HighlightingStreamConverterConfigNode$node$4$2.invalidated(HighlightingStreamConverterConfigNode.kt:129)
    at org.janelia.saalfeldlab.fx.ObservableWithListenersList.stateChanged(ObservableWithListenersList.kt:51)
    at org.janelia.saalfeldlab.paintera.stream.AbstractHighlightingARGBStream.setSeed(AbstractHighlightingARGBStream.java:150)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.changeStreamSeed(ARGBStreamSeedSetter.kt:34)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.incrementStreamSeed(ARGBStreamSeedSetter.kt:26)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.incrementStreamSeed$default(ARGBStreamSeedSetter.kt:26)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter$incrementHandler$1.handle(ARGBStreamSeedSetter.kt:15)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter$incrementHandler$1.handle(ARGBStreamSeedSetter.kt:10)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$AnyHandler.handle(DelegateEventHandlers.kt:99)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$ListDelegateEventHandler.handle(DelegateEventHandlers.kt:72)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler$handle$1.accept(DelegateEventHandlers.kt:55)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler$handle$1.accept(DelegateEventHandlers.kt:48)
    at java.util.Optional.ifPresent(Optional.java:159)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler.handle(DelegateEventHandlers.kt:55)
    at com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:218)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.Scene$KeyHandler.process(Scene.java:3964)
    at javafx.scene.Scene$KeyHandler.access$1800(Scene.java:3910)
    at javafx.scene.Scene.impl_processKeyEvent(Scene.java:2040)
    at javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2501)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:217)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:149)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$1(GlassViewEventHandler.java:248)
    at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:389)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:247)
    at com.sun.glass.ui.View.handleKeyEvent(View.java:546)
    at com.sun.glass.ui.View.notifyKey(View.java:966)
    at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at com.sun.glass.ui.gtk.GtkApplication.lambda$null$10(GtkApplication.java:245)
    at java.lang.Thread.run(Thread.java:748)
Exception in thread "JavaFX Application Thread" java.lang.IndexOutOfBoundsException: Index: 6, Size: 6
    at java.util.ArrayList.rangeCheck(ArrayList.java:657)
    at java.util.ArrayList.get(ArrayList.java:433)
    at org.janelia.saalfeldlab.fx.ObservableWithListenersList.stateChanged(ObservableWithListenersList.kt:51)
    at org.janelia.saalfeldlab.paintera.stream.AbstractHighlightingARGBStream.setSeed(AbstractHighlightingARGBStream.java:150)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.changeStreamSeed(ARGBStreamSeedSetter.kt:34)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.incrementStreamSeed(ARGBStreamSeedSetter.kt:26)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter.incrementStreamSeed$default(ARGBStreamSeedSetter.kt:26)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter$incrementHandler$1.handle(ARGBStreamSeedSetter.kt:15)
    at org.janelia.saalfeldlab.paintera.stream.ARGBStreamSeedSetter$incrementHandler$1.handle(ARGBStreamSeedSetter.kt:10)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$AnyHandler.handle(DelegateEventHandlers.kt:99)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$ListDelegateEventHandler.handle(DelegateEventHandlers.kt:72)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler$handle$1.accept(DelegateEventHandlers.kt:55)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler$handle$1.accept(DelegateEventHandlers.kt:48)
    at java.util.Optional.ifPresent(Optional.java:159)
    at org.janelia.saalfeldlab.fx.event.DelegateEventHandlers$SupplierDelegateEventHandler.handle(DelegateEventHandlers.kt:55)
    at com.sun.javafx.event.CompositeEventHandler$NormalEventHandlerRecord.handleBubblingEvent(CompositeEventHandler.java:218)
    at com.sun.javafx.event.CompositeEventHandler.dispatchBubblingEvent(CompositeEventHandler.java:80)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:238)
    at com.sun.javafx.event.EventHandlerManager.dispatchBubblingEvent(EventHandlerManager.java:191)
    at com.sun.javafx.event.CompositeEventDispatcher.dispatchBubblingEvent(CompositeEventDispatcher.java:59)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:58)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.BasicEventDispatcher.dispatchEvent(BasicEventDispatcher.java:56)
    at com.sun.javafx.event.EventDispatchChainImpl.dispatchEvent(EventDispatchChainImpl.java:114)
    at com.sun.javafx.event.EventUtil.fireEventImpl(EventUtil.java:74)
    at com.sun.javafx.event.EventUtil.fireEvent(EventUtil.java:54)
    at javafx.event.Event.fireEvent(Event.java:198)
    at javafx.scene.Scene$KeyHandler.process(Scene.java:3964)
    at javafx.scene.Scene$KeyHandler.access$1800(Scene.java:3910)
    at javafx.scene.Scene.impl_processKeyEvent(Scene.java:2040)
    at javafx.scene.Scene$ScenePeerListener.keyEvent(Scene.java:2501)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:217)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler$KeyEventNotification.run(GlassViewEventHandler.java:149)
    at java.security.AccessController.doPrivileged(Native Method)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.lambda$handleKeyEvent$1(GlassViewEventHandler.java:248)
    at com.sun.javafx.tk.quantum.QuantumToolkit.runWithoutRenderLock(QuantumToolkit.java:389)
    at com.sun.javafx.tk.quantum.GlassViewEventHandler.handleKeyEvent(GlassViewEventHandler.java:247)
    at com.sun.glass.ui.View.handleKeyEvent(View.java:546)
    at com.sun.glass.ui.View.notifyKey(View.java:966)
    at com.sun.glass.ui.gtk.GtkApplication._runLoop(Native Method)
    at com.sun.glass.ui.gtk.GtkApplication.lambda$null$10(GtkApplication.java:245)
    at java.lang.Thread.run(Thread.java:748)

I assume that this binding to the color from the stream is the culprit somehow: https://github.com/saalfeldlab/paintera/blob/bf9353b59dfd322a1eaabcdcc368e2ccea41359e/src/main/kotlin/org/janelia/saalfeldlab/paintera/meshes/managed/MeshManagerWithAssignmentForSegments.kt#L207-L211

hanslovsky commented 4 years ago

The color seed issue with exceptions when changing the seed was resolved in 7b39bad

hanslovsky commented 4 years ago

I will address the remaining issues regarding serialization of the mesh enabled toggle and add a UI toggle to turn on/off meshes for IntersectingSourceState. After that, this PR should be ready for testing. (see checklist for details)

hanslovsky commented 4 years ago

This is ready for testing now

igorpisarev commented 4 years ago

@hanslovsky I'm testing this branch now, and mostly it works fine. However, there still seems to be something wrong with the mesh progress bars while it works as expected in the master branch:

hanslovsky commented 4 years ago

Thanks for testing and helping me debug @igorpisarev Summary:

  1. MeshManagerWithAssignmentForSegments adds an InvalidationListener to SelectedSegments that enqueues a task to update the meshes whenever assignment/selection changes. This task runs on a separate thread and waits until any previous (and canceled) task has finished.
  2. MeshInfos adds an InvalidationListener to SelectedSegments that updates the meshes and requests the mesh state from the mesh manager. The mesh manager, however, may not have a state for the requested id yet, because the update task runs in a separate thread (1).

As a solution:

  1. Add a call back to the end of MeshManagerWithAssignmentForSegments.setMeshesToSelectionImpl to trigger the update of MeshInfos.
  2. In MeshInfos, do not attach the updateMeshInfosHandler to selectedSegments but to meshManager instead. Within updateMeshInfosHandler, use Platform.runLater as needed and appropriate.
hanslovsky commented 4 years ago

@igorpisarev I updated in 7824a53 with the changes we discussed last night with a few minor modifications:

  1. Use and expose anonymous object that extends ObservableWithListenersList instead of having the mesh manager extend ObservableWithListenersList. This will allow us to be more flexible if necessary
  2. No call to Platform.runLater in the updateMeshInfosHandler of MeshInfos. This does not seem to be necessary and I tested successfully without that call.

Can you test on sub-144? I only have CREMI on my laptop.

Other than that, I think this is ready to merge. Based on all the feedback I had, there were no other issues than the mesh progresses. I am still thinking about the coloring of the progress bars, though. I am not sure that the orange/green combination is the best choice.

hanslovsky commented 4 years ago

I tried a few more coloring options (in MeshProgressBar.java):

private static class ProgressStyle {
    // green (finished) and orange, OK but not great
//      public static final String COLOR_FINISHED = "green";
//      public static final String COLOR_IN_PROGRESS = "#ff7700"; // orange

    // some ideas taken from
    // https://www.designwizard.com/blog/design-trends/colour-combination

    // gray (finished) and lime punch, looks great
//      private static final String COLOR_FINISHED = "#606060FF";
//      private static final String COLOR_IN_PROGRESS = "#D6ED17FF";

    // pacific coast (finished) and living coral, looks great
    private static final String COLOR_FINISHED = "#5B84B1FF";
    private static final String COLOR_IN_PROGRESS = "#FC766AFF";

    // black/dark gray (finished) and pale orange, looks OK but I think gray/lime punch is a better combo
//      private static final String COLOR_FINISHED = "#606060FF";
//      private static final String COLOR_IN_PROGRESS = "#F2AA4CFF";

    // charcoal gray (finished) and pink salt, looks great but may be too pastel for some tastes
//      private static final String COLOR_FINISHED = "#6E6E6DFF";
//      private static final String COLOR_IN_PROGRESS = "#FAD0C9FF";

    // gray (finished) and CREMI magenta, CREMI too bold for my liking
//      private static final String COLOR_FINISHED = "#606060FF";
//      private static final String COLOR_IN_PROGRESS = Colors.toHTML(Colors.CREMI);

    private static String getStyle(final String color) {
        return String.format("-fx-accent: %s; ", color);
    }

    public static final String IN_PROGRESS = getStyle(COLOR_IN_PROGRESS);
    public static final String FINISHED = getStyle(COLOR_FINISHED);
}

I looked at https://www.designwizard.com/blog/design-trends/colour-combination for insipriation for colors. My current favorites are pacific coast/living coral and gray/lime punch. If no other opinions are voiced, I will choose between those two options.

igorpisarev commented 4 years ago

@hanslovsky I tried the updated branch on sub-144, and it's now working as expected there as well.

I also did some quick benchmarking, and somehow this branch outperforms the current master quite significantly: 1:05 to generate all meshes in sub-144 in this branch vs. 1:40 in the current master, and it's consistent across several runs. I don't know if it's because of slightly different/relaxed synchronization in the mesh manager or something else, but it's pretty good! (I used the same mesh settings and the lowest resolution, total number of blocks is 117260).

Regarding the progress bar colors, my personal preference is probably the current master where the progress bar is green and is displayed only when there are still pending tasks (I feel like it's the least redundant behavior), but I'm also fine with the current color scheme.

hanslovsky commented 4 years ago

Thanks for testing again and the benchmark @igorpisarev

I decided to go with two different colors because I find it more intuitive to distinguish between completion of mesh generation and "nothing happens" that way. Once CI has passed, I will go ahead and merge.

hanslovsky commented 4 years ago

Merging this PR is delayed because I am currently evaluating JVM crashes on this branch with @cpatrickc. They may be unrelated to this PR but I want to make sure that this assumption is correct.

Example log: hs_err_pid23371.log

Possibly related bugs: https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8029679 https://bugs.java.com/bugdatabase/view_bug.do?bug_id=JDK-8028764 https://bugs.openjdk.java.net/browse/JDK-8163436?focusedCommentId=13982722&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-13982722

If this is an actual garbage collector crash, it will be really hard to figure out the cause and it will probably not be related to this PR. @cpatrickc did not specify a garbage collector when executing Paintera, thus defaulting to Java 8's default parallel GC. paintera-cmd uses CMS by default, which may explain why this issue has not come up before.

oflynne commented 4 years ago
java.util.concurrent.ExecutionException: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 526
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:112)
    at net.imglib2.cache.util.LoaderCacheAsCacheAdapter.get(LoaderCacheAsCacheAdapter.java:43)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache.getMeshFor(GetMeshFor.kt:17)
    at org.janelia.saalfeldlab.paintera.meshes.MeshGeneratorJobManager.lambda$createTask$10(MeshGeneratorJobManager.java:499)
    at org.janelia.saalfeldlab.paintera.meshes.MeshGeneratorJobManager.lambda$withErrorPrinting$35(MeshGeneratorJobManager.java:1154)
    at org.janelia.saalfeldlab.util.concurrent.HashPriorityQueueBasedTaskExecutor.runWorker(HashPriorityQueueBasedTaskExecutor.java:146)
    at org.janelia.saalfeldlab.util.concurrent.HashPriorityQueueBasedTaskExecutor.lambda$null$0(HashPriorityQueueBasedTaskExecutor.java:43)
    at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 526
    at net.imglib2.cache.util.CacheAsUncheckedCacheAdapter.get(CacheAsUncheckedCacheAdapter.java:45)
    at net.imglib2.img.cell.LazyCellImg$LazyCells.get(LazyCellImg.java:104)
    at net.imglib2.img.list.AbstractLongListImg$LongListRandomAccess.get(AbstractLongListImg.java:274)
    at net.imglib2.img.cell.CellRandomAccess.getCell(CellRandomAccess.java:136)
    at net.imglib2.img.cell.CellRandomAccess.updatePosition(CellRandomAccess.java:474)
    at net.imglib2.img.cell.CellRandomAccess.fwd(CellRandomAccess.java:164)
    at net.imglib2.converter.AbstractConvertedRandomAccess.fwd(AbstractConvertedRandomAccess.java:110)
    at net.imglib2.view.RandomAccessibleIntervalCursor.fwd(RandomAccessibleIntervalCursor.java:115)
    at net.imglib2.view.RandomAccessibleIntervalCursor.next(RandomAccessibleIntervalCursor.java:150)
    at org.janelia.saalfeldlab.paintera.meshes.MarchingCubes.generateMesh(MarchingCubes.java:481)
    at org.janelia.saalfeldlab.paintera.meshes.cache.AbstractMeshCacheLoader.get(AbstractMeshCacheLoader.java:67)
    at org.janelia.saalfeldlab.paintera.meshes.cache.AbstractMeshCacheLoader.get(AbstractMeshCacheLoader.java:24)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$fromPairLoaders$1.get(GetMeshFor.kt:48)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$fromPairLoaders$1.get(GetMeshFor.kt:19)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$asPainteraTriangleMeshLoader$1.get(GetMeshFor.kt:52)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$asPainteraTriangleMeshLoader$1.get(GetMeshFor.kt:19)
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:102)
    ... 7 more
Caused by: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 526
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:112)
    at net.imglib2.cache.util.LoaderCacheAsCacheAdapter.get(LoaderCacheAsCacheAdapter.java:43)
    at net.imglib2.cache.util.CacheAsUncheckedCacheAdapter.get(CacheAsUncheckedCacheAdapter.java:41)
    ... 23 more
Caused by: java.lang.ArrayIndexOutOfBoundsException: 526
    at gnu.trove.impl.hash.TLongHash.indexRehashed(TLongHash.java:233)
    at gnu.trove.impl.hash.TLongHash.index(TLongHash.java:219)
    at gnu.trove.impl.hash.TLongHash.contains(TLongHash.java:163)
    at org.janelia.saalfeldlab.paintera.control.selection.FragmentsInSelectedSegments.contains(FragmentsInSelectedSegments.java:47)
    at org.janelia.saalfeldlab.paintera.state.IntersectingSourceState.lambda$checkForLabelMultisetType$23(IntersectingSourceState.java:562)
    at org.janelia.saalfeldlab.paintera.state.LabelIntersectionCellLoader.load(LabelIntersectionCellLoader.java:73)
    at net.imglib2.cache.img.LoadedCellCacheLoader.get(LoadedCellCacheLoader.java:91)
    at net.imglib2.cache.img.LoadedCellCacheLoader.get(LoadedCellCacheLoader.java:51)
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:102)
    ... 25 more
Early termination of task {shapeId={261206,399427,333222,206630,207735,206626,562569,216318,216317,216316,261465,399721,751751,350649,347464,216301,346490,216298,333354,751457,751451,543700,1963861,216283,216282,208525,375270,331501,331500,206847,541738,206841,543671,399353,333425,399627,182194,181902,751413,751964,181895,562491,333133,348366,261384,216224,350567,206514,206684,333389,283118,332555,754425,206516,208093,261146,399289,399505,402067,206778,541669,519508,519507,182397,182395,182394,728635,401958,728633,402059,751345,216179,562428,751895,517828,181824,261455,541641,544130,182090,207019,185135,261309,261584,283189,282909,216104,216253,184570,206729,402013,333316,402009,728634,731083,157654,399511,182341,182339,182337,182336,346596,205879,401994,346593,371803,206983,182051,182089,182047,154899,207801,261259,206411,154887,259315,184799,333272,216100,184800,184797,216097,206678,216094,261527,261526,154874,307777,261531,401954,216084,216907,346541,350695,181999,332683,282822,181991}, scaleIndex=3, simplifications=0, smoothingLambda=0.50, smoothings=5, minLabelRatio=0.25, min=[240, 176, 208], max=[255, 191, 223]}
java.util.concurrent.ExecutionException: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 2613
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:112)
    at net.imglib2.cache.util.LoaderCacheAsCacheAdapter.get(LoaderCacheAsCacheAdapter.java:43)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache.getMeshFor(GetMeshFor.kt:17)
    at org.janelia.saalfeldlab.paintera.meshes.MeshGeneratorJobManager.lambda$createTask$10(MeshGeneratorJobManager.java:499)
    at org.janelia.saalfeldlab.paintera.meshes.MeshGeneratorJobManager.lambda$withErrorPrinting$35(MeshGeneratorJobManager.java:1154)
    at org.janelia.saalfeldlab.util.concurrent.HashPriorityQueueBasedTaskExecutor.runWorker(HashPriorityQueueBasedTaskExecutor.java:146)
    at org.janelia.saalfeldlab.util.concurrent.HashPriorityQueueBasedTaskExecutor.lambda$null$0(HashPriorityQueueBasedTaskExecutor.java:43)
    at java.lang.Thread.run(Thread.java:748)
Caused by: java.lang.RuntimeException: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 2613
    at net.imglib2.cache.util.CacheAsUncheckedCacheAdapter.get(CacheAsUncheckedCacheAdapter.java:45)
    at net.imglib2.img.cell.LazyCellImg$LazyCells.get(LazyCellImg.java:104)
    at net.imglib2.img.list.AbstractLongListImg$LongListRandomAccess.get(AbstractLongListImg.java:274)
    at net.imglib2.img.cell.CellRandomAccess.getCell(CellRandomAccess.java:136)
    at net.imglib2.img.cell.CellRandomAccess.updatePosition(CellRandomAccess.java:474)
    at net.imglib2.img.cell.CellRandomAccess.fwd(CellRandomAccess.java:164)
    at net.imglib2.converter.AbstractConvertedRandomAccess.fwd(AbstractConvertedRandomAccess.java:110)
    at net.imglib2.view.RandomAccessibleIntervalCursor.fwd(RandomAccessibleIntervalCursor.java:115)
    at net.imglib2.view.RandomAccessibleIntervalCursor.next(RandomAccessibleIntervalCursor.java:150)
    at org.janelia.saalfeldlab.paintera.meshes.MarchingCubes.generateMesh(MarchingCubes.java:483)
    at org.janelia.saalfeldlab.paintera.meshes.cache.AbstractMeshCacheLoader.get(AbstractMeshCacheLoader.java:67)
    at org.janelia.saalfeldlab.paintera.meshes.cache.AbstractMeshCacheLoader.get(AbstractMeshCacheLoader.java:24)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$fromPairLoaders$1.get(GetMeshFor.kt:48)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$fromPairLoaders$1.get(GetMeshFor.kt:19)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$asPainteraTriangleMeshLoader$1.get(GetMeshFor.kt:52)
    at org.janelia.saalfeldlab.paintera.meshes.managed.GetMeshFor$FromCache$Companion$asPainteraTriangleMeshLoader$1.get(GetMeshFor.kt:19)
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:102)
    ... 7 more
Caused by: java.util.concurrent.ExecutionException: java.lang.ArrayIndexOutOfBoundsException: 2613
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:112)
    at net.imglib2.cache.util.LoaderCacheAsCacheAdapter.get(LoaderCacheAsCacheAdapter.java:43)
    at net.imglib2.cache.util.CacheAsUncheckedCacheAdapter.get(CacheAsUncheckedCacheAdapter.java:41)
    ... 23 more
Caused by: java.lang.ArrayIndexOutOfBoundsException: 2613
    at gnu.trove.impl.hash.TLongHash.indexRehashed(TLongHash.java:233)
    at gnu.trove.impl.hash.TLongHash.index(TLongHash.java:219)
    at gnu.trove.impl.hash.TLongHash.contains(TLongHash.java:163)
    at org.janelia.saalfeldlab.paintera.control.selection.FragmentsInSelectedSegments.contains(FragmentsInSelectedSegments.java:47)
    at org.janelia.saalfeldlab.paintera.state.IntersectingSourceState.lambda$checkForLabelMultisetType$23(IntersectingSourceState.java:562)
    at org.janelia.saalfeldlab.paintera.state.LabelIntersectionCellLoader.load(LabelIntersectionCellLoader.java:73)
    at net.imglib2.cache.img.LoadedCellCacheLoader.get(LoadedCellCacheLoader.java:91)
    at net.imglib2.cache.img.LoadedCellCacheLoader.get(LoadedCellCacheLoader.java:51)
    at net.imglib2.cache.ref.SoftRefLoaderCache.get(SoftRefLoaderCache.java:102)
    ... 25 more
Early termination of task {shapeId={261206,399427,333222,206630,207735,206626,562569,216318,216317,216316,261465,399721,751751,350649,347464,216301,346490,216298,333354,751457,751451,543700,1963861,216283,216282,208525,375270,331501,331500,206847,541738,206841,543671,399353,333425,399627,182194,181902,751413,751964,181895,562491,333133,348366,261384,216224,350567,206514,206684,333389,283118,332555,754425,206516,208093,261146,399289,399505,402067,206778,541669,519508,519507,182397,182395,182394,728635,401958,728633,402059,751345,216179,562428,751895,517828,181824,261455,541641,544130,182090,207019,185135,261309,261584,283189,282909,216104,216253,184570,206729,402013,333316,402009,728634,731083,157654,399511,182341,182339,182337,182336,346596,205879,401994,346593,371803,206983,182051,182089,182047,154899,207801,261259,206411,154887,259315,184799,333272,216100,184800,184797,216097,206678,216094,261527,261526,154874,307777,261531,401954,216084,216907,346541,350695,181999,332683,282822,181991}, scaleIndex=3, simplifications=0, smoothingLambda=0.50, smoothings=5, minLabelRatio=0.25, min=[240, 128, 176], max=[255, 143, 191]}
hanslovsky commented 4 years ago

The stack trace looks like the fragment selection is modified while, at the same time on a different thread, a contains check is run for a fragment id. A solution would be to remove the final modifier of FragmentsInSelectedSegments.selectedFragments and, instead of modifying FragmentsInSelectedSegments.selectedFragments in place in FragmentsInSelectedSegments.update, replace the reference at each call. @igorpisarev can you try that with @oflynne? Thank you!

The other option would be to synchronize FragmentsInSelectedSegments.contains but that defeats the purpose because it will make everything very slow.

igorpisarev commented 4 years ago

@hanslovsky That's a good idea, replacing the reference should do it -- it's probably fine if another thread gets slightly outdated result for contains() (if the reference is replaced by another thread while it's still running). I can push this change and we'll try it out.

igorpisarev commented 4 years ago

I've pushed the fix as @hanslovsky suggested, and it looks like the exceptions are gone. Also this issue seems to be the same problem and is fixed now: #344

hanslovsky commented 4 years ago

Thanks @igorpisarev

hanslovsky commented 4 years ago

@oflynne reported that setting minLabelRatio=0 is much faster. Maybe there are ways to improve speed with minLabelRatio>0: https://github.com/saalfeldlab/paintera/blob/074c2279da9c80844961b3e555e7dbfea9706404/src/main/java/org/janelia/saalfeldlab/paintera/meshes/cache/SegmentMaskGenerators.java#L108-L146

hanslovsky commented 4 years ago

Not really a benchmark and I had to make SegmentMaskGenerators.LabelMultisetTypeMask public, but it confirms what we observe (with many valid fragments and minLabelRatio>0, the mask is very slow):

package org.janelia.saalfeldlab.paintera.meshes

import gnu.trove.set.hash.TLongHashSet
import net.imglib2.type.label.LabelMultisetEntry
import net.imglib2.type.label.LabelMultisetEntryList
import net.imglib2.type.label.LabelMultisetType
import net.imglib2.type.logic.BoolType
import org.janelia.saalfeldlab.paintera.meshes.cache.SegmentMaskGenerators
import kotlin.math.min
import kotlin.time.measureTime

@kotlin.time.ExperimentalTime
fun main() {

    val validLabels1 = TLongHashSet((0L until 2L).toList().toLongArray())
    val validLabels2 = TLongHashSet((0L until 10L).toList().toLongArray())
    val validLabels3 = TLongHashSet((0L until 10_000L).toList().toLongArray())

    val numFullResPixels = 64L

    var remaining = numFullResPixels
    var id = 1L
    val entries = LabelMultisetEntryList()
    while (remaining > 0) {
        val count = min(remaining / 2 + 1, remaining)
        // println("$remaining $count")
        entries.add(LabelMultisetEntry(id, count.toInt()))
        remaining -= count
        ++id
    }

    val source = LabelMultisetType(entries)
    val target = BoolType()
    val times = 100_000

    doubleArrayOf(0.0, 0.25).forEach { mlr ->
        val mg1 = SegmentMaskGenerators.LabelMultisetTypeMask<BoolType>(validLabels1, mlr, numFullResPixels)
        val mg2 = SegmentMaskGenerators.LabelMultisetTypeMask<BoolType>(validLabels2, mlr, numFullResPixels)
        val mg3 = SegmentMaskGenerators.LabelMultisetTypeMask<BoolType>(validLabels3, mlr, numFullResPixels)
        repeat(times) { mg1.convert(source, target) }
        val t1 = measureTime { repeat(times) { mg1.convert(source, target) } }
        repeat(times) { mg2.convert(source, target) }
        val t2 = measureTime { repeat(times) { mg2.convert(source, target) } }
        repeat(times) { mg3.convert(source, target) }
        val t3 = measureTime { repeat(times) { mg3.convert(source, target) } }
        println("For mlr=$mlr: t1=$t1 t2=$t2 t3=$t3")
    }

}

Output:

For mlr=0.0: t1=11.0ms t2=7.46ms t3=7.97ms
For mlr=0.25: t1=36.6ms t2=64.1ms t3=38.3s
igorpisarev commented 4 years ago

I pushed another minor optimization for minLabelRatio>0 as I realized there was another missed opportunity which should slightly improve performance in average case, but the improvement is probably rather small compared to the previous fix.

hanslovsky commented 4 years ago

@igorpisarev minNumRequiredPixels can be initialized at construction time, can it not?

Also, we could split the class into two cases:

  1. `minLabelRatio == null || minLabelRatio == 0.0
  2. All other cases That would save us a branching condition within the conversion

What do you think?

hanslovsky commented 4 years ago

I applied the suggested changes in 65eb9cc53d6afab61fafad6ff9ba1ce1f56fbae9 Let me know what you think @igorpisarev If there is something that I was not aware of, we can always revert.

hanslovsky commented 4 years ago

FWIW, here are my "benchmark" results after separating out into two classes:

For mlr=0.0: t1=13.5ms t2=8.57ms t3=8.27ms
For mlr=0.25: t1=20.8ms t2=10.3ms t3=7.90ms

And this is what I measured before:

For mlr=0.0: t1=11.2ms t2=6.79ms t3=10.9ms
For mlr=0.25: t1=24.6ms t2=101ms t3=14.1ms

This is not a real benchmark but just gives a rule of thumbs estimate. I do not think that we need a real benchmark on this, though.

hanslovsky commented 4 years ago

We could even go further and separate out into more classes to avoid the branching at if (validLabelsSize < inputSize) but that may not have as big an impact because we would have to use a heuristic: At each level, we know the maximum number of possible entries in a multiset: It is the maximum number of entries at the previous level mulitplied with the product of relative downsampling factors or just the number of maxinum number of entries at the current level, whichever is smaller. If we use this as an upper bound, we could just make a decision based on that at the time of construction rather than compare to validLabelsSize at every voxel. The maximum number of entries will be relatively small in most use cases, so it should be a fairly good heuristic. The cost of this would be two additional classes and inlining may not happen as much anymore.

igorpisarev commented 4 years ago

@hanslovsky cool, that's another nice optimization!

Not sure how much of a performance boost splitting into more classes based on the set size would give, but it may be interesting to try as well

hanslovsky commented 4 years ago

Not sure how much of a performance boost splitting into more classes based on the set size would give, but it may be interesting to try as well

I tried to write a more serious benchmark but I was in unable to get jmh to work for some reason. That made me think that splitting into more classes is probably over-optimizing and probably not worth the gains, it there are any. So right now I am inclined to just forego splitting the classes any further. What do you think?

In particular as performance is already good enough, as it seems

igorpisarev commented 4 years ago

I'm happy with the current performance too, so I agree that it should be good to go as it is.

hanslovsky commented 4 years ago

Should we merge @igorpisarev? I think this has been tested quite extensively. If you are ok with it, I will go ahead and merge and we can cut a release after that.

igorpisarev commented 4 years ago

Sounds good to me!