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.

Sign up for GitHub

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

Jump to bottom

Fix rendering if 3D viewport is hidden initially #5429

Merged
merged 3 commits into from
Apr 29, 2021
Merged

Fix rendering if 3D viewport is hidden initially #5429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2af6581
fix rendering if 3D viewport is hidden initially
philippotto Apr 27, 2021
06fd55a
eagerly render camera controller in td controller; default to empty v…
philippotto Apr 28, 2021
8d9c3ed
update changelog
philippotto Apr 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Fixed a bug where the page would scroll up unexpectedly when showing various confirm modals. [#5371](https://github.com/scalableminds/webknossos/pull/5371)
- Fixed a bug where user changes (email, activation) would show as successful even if they actually failed. [#5392](https://github.com/scalableminds/webknossos/pull/5392)
- Fixed a bug where dataset uploads were sent to the wrong datastore, and failed [#5404](https://github.com/scalableminds/webknossos/pull/5404)
- Fixed a rendering bug which occurred when the initial layout had a hidden 3D viewport. [#5429](https://github.com/scalableminds/webknossos/pull/5429)

### Removed
-
Expand Down
21 changes: 13 additions & 8 deletions frontend/javascripts/oxalis/controller/td_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ function maybeGetActiveNodeFromProps(props: Props) {
}

class TDController extends React.PureComponent<Props> {
controls: typeof TrackballControls;
controls: ?typeof TrackballControls;
mouseController: InputMouse;
oldNmPos: Vector3;
isStarted: boolean;
Expand Down Expand Up @@ -148,7 +148,12 @@ class TDController extends React.PureComponent<Props> {
this.forceUpdate();
}

updateControls = () => this.controls.update(true);
updateControls = () => {
if (!this.controls) {
return;
}
this.controls.update(true);
};

getTDViewMouseControls(): Object {
const skeletonControls =
Expand Down Expand Up @@ -207,11 +212,15 @@ class TDController extends React.PureComponent<Props> {
}

setTargetAndFixPosition(position?: Vector3): void {
const { controls } = this;
if (controls == null) {
return;
}
position = position || getPosition(this.props.flycam);
const nmPosition = voxelToNm(this.props.scale, position);

this.controls.target.set(...nmPosition);
this.controls.update();
controls.target.set(...nmPosition);
controls.update();

// The following code is a dirty hack. If someone figures out
// how the trackball control's target can be set without affecting
Expand Down Expand Up @@ -260,10 +269,6 @@ class TDController extends React.PureComponent<Props> {
}

render() {
if (!this.controls) {
return null;
}
Comment on lines -263 to -265
Copy link
Member Author

Choose a reason for hiding this comment

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

By postponing this null-check to the exact points where it's necessary (i.e., in setTargetAndFixPosition), we can eagerly instantiate the CameraController which should fix the issue.


return (
<CameraController
cameras={this.props.cameras}
Expand Down
27 changes: 12 additions & 15 deletions frontend/javascripts/oxalis/controller/viewmodes/plane_controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,21 +134,18 @@ class PlaneController extends React.PureComponent<Props> {
// catchers (only necessary for HammerJS). We should refactor the
// InputMouse handling so that this is not necessary anymore.
// See: https://github.com/scalableminds/webknossos/issues/3475
const tdId = `inputcatcher_${OrthoViews.TDView}`;
Utils.waitForElementWithId(tdId).then(() => {
OrthoViewValuesWithoutTDView.forEach(id => {
const inputcatcherId = `inputcatcher_${OrthoViews[id]}`;
Utils.waitForElementWithId(inputcatcherId).then(el => {
if (!document.body.contains(el)) {
console.error("el is not attached anymore");
}
this.input.mouseControllers[id] = new InputMouse(
inputcatcherId,
this.getPlaneMouseControls(id),
id,
true,
);
});
OrthoViewValuesWithoutTDView.forEach(id => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Waiting for the 3d viewport did not succeed here, if it's hidden initially. In that case, the mouse handlers were not set up. Since, there is a waitForElementWithId two lines below for each viewport, I'd hope that this change doesn't break anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this won't be a problem, as the waitForElementWithId works with promises. The onliest thing I see is that if one of the viewports is never rendered, that there will be endless calls of window.requestAnimationFrame as long as wk is opened in a browser tab.
I think this is not optimal. Maybe we can add a timeout to this function and retrigger adding the mouse controllers for all inputcatchers that were newly rendered when the layout is changed.
But this would be a thing for another PR.

Note:

export function waitForElementWithId(elementId: string): Promise<*> {
  const tryToResolve = resolve => {
    const el = document.getElementById(elementId);
    if (el) {
      resolve(el);
    } else {
      window.requestAnimationFrame(() => tryToResolve(resolve));
    }
  };
  return new Promise(tryToResolve);
}

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 point! I cross-referenced this comment in the corresponding issue: #3475

I agree that this is out-of-scope for this PR.

const inputcatcherId = `inputcatcher_${OrthoViews[id]}`;
Utils.waitForElementWithId(inputcatcherId).then(el => {
if (!document.body.contains(el)) {
console.error("el is not attached anymore");
}
this.input.mouseControllers[id] = new InputMouse(
inputcatcherId,
this.getPlaneMouseControls(id),
id,
true,
);
});
});
}
Expand Down
2 changes: 1 addition & 1 deletion frontend/javascripts/oxalis/default_state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import type { OxalisState } from "oxalis/store";
import Constants, { ControlModeEnum, OrthoViews, OverwriteModeEnum } from "oxalis/constants";

const defaultViewportRect = {
export const defaultViewportRect = {
top: 0,
left: 0,
width: Constants.VIEWPORT_WIDTH,
Expand Down
15 changes: 14 additions & 1 deletion frontend/javascripts/oxalis/view/input_catcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ import ViewportStatusIndicator from "oxalis/view/viewport_status_indicator";
import Store from "oxalis/store";
import makeRectRelativeToCanvas from "oxalis/view/layouting/layout_canvas_adapter";

const emptyViewportRect = {
top: 0,
left: 0,
width: 0,
height: 0,
};

type Props = {
viewportID: Viewport,
children?: React.Node,
Expand Down Expand Up @@ -48,7 +55,13 @@ function adaptInputCatcher(inputCatcherDOM: HTMLElement, makeQuadratic: boolean)
const renderedInputCatchers = new Map();

export function recalculateInputCatcherSizes() {
const viewportRects = {};
const viewportRects: Object = {
philippotto marked this conversation as resolved.
Show resolved Hide resolved
PLANE_XY: emptyViewportRect,
PLANE_YZ: emptyViewportRect,
PLANE_XZ: emptyViewportRect,
TDView: emptyViewportRect,
};

for (const [viewportID, inputCatcher] of renderedInputCatchers.entries()) {
const makeQuadratic = viewportID === ArbitraryViewport;
const rect = adaptInputCatcher(inputCatcher, makeQuadratic);
Expand Down