search

saalfeldlab / paintera

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

Exclude all special value labels when committing canvas #305

Closed hanslovsky closed 5 years ago

hanslovsky commented 5 years ago

Fixes #294

We currently have special values

TRANSPARENT = -0x1L // -1L or uint64.MAX_VALUE
INVALID = -0x2L // -2L or uint64.MAX_VALUE - 1
OUTSIDE = -0x3L // -3L or uint64.MAX_VALUE - 2
MAX_ID = -0x4L // -4L or uint64.MAX_VALUE - 3

@axtimwalde @igorpisarev do you see any use case where we should actually paint a special value? In that case, this check would be to aggressive.

igorpisarev commented 5 years ago

The fix makes sense to me -- can't come up with a case when committing these values would be needed

hanslovsky commented 5 years ago

This breaks the BackgroundCanvasIterableTest. Maybe #294 is not actually a bug. It depends on what "erase" means: Does it erase from the canvas or does it erase from the background? On current master: erase from background On this PR: erase from canvas

It may be useful to actually erase from background, e.g. when no valid foreground or background label is applicable. There are two ways forward here:

  1. Accept this PR and change the failing test and say that erasing is erasing from background only
  2. Close this PR and add another event handler instead that erases from canvas instead of background.

Currently I am in favor of (2) but I'll let this sit for a while.

hanslovsky commented 5 years ago

For this discussion it would probably best to hear @wangyuhan01's opinion: When you erase,

Do you

  1. want to erase what you painted into the canvas, i.e. make sure that the erase areas will not be committed into the background (all voxels in the data are either background or a valid foreground id), or
  2. erase in the background, i.e. set an "unknown" or "not processed yet" for the affected voxels when committing (current behavior), or
  3. both of (1) and (2)?
wangyuhan01 commented 5 years ago

Thanks so much @hanslovsky for looking into this issue. Option (1) will suffice, for us each voxel is either background or foreground with id.

hanslovsky commented 5 years ago

I updated the test to not fail under the assumption that erasing means erasing from the canvas (option (1) above). If, in the future, it may become desirable to actually erase in the background, this should be revisited.

hanslovsky commented 5 years ago

@wangyuhan01 Paintera 0.19.2 with this bug fix is now availble through conda and PyPI.