Dont brush outside viewport #3283
Conversation
…/webknossos into experience-field-as-select
…erience-field-as-select
…t-brush-outside-viewport
…s/webknossos into dont-brush-outside-viewport
| const zoom = yield* select(state => state.flycam.zoomStep); | ||
| const scales = yield* select(state => getBaseVoxelFactors(state.dataset.dataSource.scale)); | ||
| const halfViewportWidth = Math.round((Constants.PLANE_WIDTH / 2) * zoom); | ||
| let relevantCoordinates; |
There was a problem hiding this comment.
const relevantCoordinates = transDim([1, 1, 0]) should work here instead of the switch block.
| @@ -96,13 +98,17 @@ export function* editVolumeLayerAsync(): Generator<any, any, any> { | |||
| if (!addToLayerAction || addToLayerAction.type !== "ADD_TO_LAYER") { | |||
| throw new Error("Unexpected action. Satisfy flow."); | |||
| } | |||
|
|
|||
| // if the current viewport does not match the initial viewport -> dont draw | |||
| if (initialViewport !== (yield* select(state => state.viewModeData.plane.activeViewport))) { | |||
There was a problem hiding this comment.
I'd extract the right-hand side into a activeViewport variable and move the comment into the if-block above the continue statement.
| function* getBoundingsFromPosition(currentViewport: OrthoView): Saga<?BoundingBox> { | ||
| const position = Dimensions.roundCoordinate(yield* select(state => getPosition(state.flycam))); | ||
| const zoom = yield* select(state => state.flycam.zoomStep); | ||
| const scales = yield* select(state => getBaseVoxelFactors(state.dataset.dataSource.scale)); |
There was a problem hiding this comment.
I'd rename scales to baseVoxelFactors or something similar since this is clearer in my opinion.
| ) { | ||
| this.map = map; | ||
| this.width = width; | ||
| this.height = height; | ||
| this.minCoord2d = minCoord2d; | ||
| this.get3DCoordinate = get3DCoordinate; | ||
| if (!this.map[0][0]) { | ||
| this.boundingBox = boundingBox; | ||
| const firstCoordinate = this.get3DCoordinate([this.minCoord2d[0], this.minCoord2d[1]]); |
There was a problem hiding this comment.
Shouldn't get3DCoordinate(this.minCoord2d) be enough?
| @@ -35,29 +37,59 @@ export class VoxelIterator { | |||
| width: number, | |||
| height: number, | |||
| minCoord2d: Vector2, | |||
| get3DCoordinate: Vector2 => Vector3 = () => [0, 0, 0], | |||
| get3DCoordinate?: Vector2 => Vector3 = () => [0, 0, 0], | |||
There was a problem hiding this comment.
I know that this has been like that before, but I think this parameter should be not optional and should not have a default value. The default value of () => [0, 0, 0] is a bit dangerous in my opinion, since there are only very few cases where this makes sense. As far as I know, this default value is only used for the static finished method of VoxelIterator. Can you remove the default value here and make it non-optional? The former default value can then be inlined within the static method.
| foundNext = true; | ||
| this.hasNext = false; | ||
| } | ||
| if (this.map[this.x][this.y]) { |
There was a problem hiding this comment.
I think you can change this to an else if since y === height means that we reached the end. So, in that case we don't need to run this block.
| if (!this.map[0][0]) { | ||
| this.boundingBox = boundingBox; | ||
| const firstCoordinate = this.get3DCoordinate([this.minCoord2d[0], this.minCoord2d[1]]); | ||
| if (!this.map[0][0] || !this.isCoordinateInBounds(firstCoordinate)) { |
There was a problem hiding this comment.
Can you invert the if condition and switch the body and else block. In my opinion, that's easier to read. So, it would be something like:
if (map[0][0] != null && isInBounds...) {
this.next = firstCoordinate;
} else {
this.getNext();
}
|
@philippotto This PR is ready for another review 😃 |
This PR disables drawing out of bounds of a viewport
URL of deployed dev instance (used for testing):
Steps to test:
Issues: