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

Allow moving layers using the keyboard arrow keys #7202

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Allow moving layers using the keyboard arrow keys #7202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
31dead0
Allow moving layers using the keyboard
hippalectryon-0 Oct 25, 2024
9f05d95
Don't move if the layer is disabled
hippalectryon-0 Oct 25, 2024
483ceb2
refactor
hippalectryon-0 Oct 25, 2024
574e390
Merge branch 'main' into keyboard_move
hippalectryon-0 Oct 25, 2024
52d3461
use $isInteractable instead of $isDisabled
hippalectryon-0 Oct 25, 2024
e596c2e
Merge remote-tracking branch 'origin/keyboard_move' into keyboard_move
hippalectryon-0 Oct 25, 2024
5e6bd2e
lint
hippalectryon-0 Oct 25, 2024
96442f9
Make sure the canvas is focused
hippalectryon-0 Oct 26, 2024
696cc37
Use proxyrect and setEntityPosition to sync transformer position
hippalectryon-0 Oct 26, 2024
5a03121
Merge branch 'main' into keyboard_move
hippalectryon-0 Oct 26, 2024
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
2 changes: 1 addition & 1 deletion invokeai/frontend/web/src/common/hooks/focus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const REGION_TARGETS: Record<FocusRegionName, Set<HTMLElement>> = {
/**
* The currently-focused region or `null` if no region is focused.
*/
const $focusedRegion = atom<FocusRegionName | null>(null);
export const $focusedRegion = atom<FocusRegionName | null>(null);

/**
* A map of focus regions to atoms that indicate if that region is focused.
Expand Down
12 changes: 8 additions & 4 deletions ...eai/frontend/web/src/features/controlLayers/konva/CanvasEntity/CanvasEntityTransformer.ts
View file
Open in desktop
Copy link
Collaborator

@psychedelicious psychedelicious Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This doesn't work quite how we need it to, because an entity's objects are positioned relative to the entity's position. For example:
Screenshot 2024-10-29 at 1 11 51 pm

Here, the entity position is 0,0, but the objects - represented by the interaction proxy rect - are at a different position.

As a result, when we use do a single-pixel nudge, we offset the entity by the object position + the single-pixel nudge offsets.

Screen.Recording.2024-10-29.at.1.24.59.pm.mov

Instead of handling this with logic in the transformer class, I think it's best to create a redux action specifically for moving an entity by a certain amount. Proposal:

Optionally, you could do something like this to immediately move the entity and its objects before waiting for the redux round-trip (untested):

nudgePosition = (offset: Coordinate) => {

  //#region Optional perf optimization
  
  // We can immediately move both the proxy rect and layer objects so we don't have to wait for a redux round-trip,
  // which can take up to 2ms in my testing. This is optional, but can make the interaction feel more responsive,
  // especially on lower-end devices.
  // Get the relative position of the layer's objects, according to konva
  const position = this.konva.proxyRect.position();
  // Offset the position by the nudge amount
  const newPosition = offsetCoord(position, offset);
  // Set the new position of the proxy rect - this doesn't move the layer objects - only the outline rect
  this.konva.proxyRect.setAttrs(newPosition);
  // Sync the layer objects with the proxy rect - moves them to the new position
  this.syncObjectGroupWithProxyRect();
  
  //#endregion
  
  // Push to redux. The state change will do a round-trip, and eventually make it back to the canvas classes, at
  // which point the layer will be moved to the new position.
  this.manager.stateApi.moveEntityBy({ entityIdentifier: this.parent.entityIdentifier, offset });
};

Original file line number Diff line number Diff line change
Expand Up @@ -540,14 +540,18 @@ export class CanvasEntityTransformer extends CanvasModuleBase {
}

const pixelRect = this.$pixelRect.get();
this.nudgePosition(-pixelRect.x, -pixelRect.y);
};

nudgePosition = (x: number, y: number) => {
// Nudge the position by (x, y) pixels
const position = {
x: this.konva.proxyRect.x() - pixelRect.x,
y: this.konva.proxyRect.y() - pixelRect.y,
x: this.konva.proxyRect.x() + x,
y: this.konva.proxyRect.y() + y,
};

this.log.trace({ position }, 'Position changed');
// Push state to redux
this.manager.stateApi.setEntityPosition({ entityIdentifier: this.parent.entityIdentifier, position });
this.log.trace({ position }, 'Position changed');
};

syncObjectGroupWithProxyRect = () => {
Expand Down
25 changes: 24 additions & 1 deletion invokeai/frontend/web/src/features/controlLayers/konva/CanvasTool/CanvasMoveToolModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { $focusedRegion } from 'common/hooks/focus';
import type { CanvasManager } from 'features/controlLayers/konva/CanvasManager';
import { CanvasModuleBase } from 'features/controlLayers/konva/CanvasModuleBase';
import type { CanvasToolModule } from 'features/controlLayers/konva/CanvasTool/CanvasToolModule';
Expand All @@ -20,12 +21,34 @@ export class CanvasMoveToolModule extends CanvasModuleBase {
this.manager = this.parent.manager;
this.path = this.manager.buildPath(this);
this.log = this.manager.buildLogger(this);

this.log.debug('Creating module');
}

/**
* This is a noop. Entity transformers handle cursor style when the move tool is active.
*/
syncCursorStyle = noop;

onKeyDown = (e: KeyboardEvent) => {
// Support moving via arrow keys
const OFFSET = 1; // How much to move, in px
const offsets: Record<string, { x: number; y: number }> = {
ArrowLeft: { x: -OFFSET, y: 0 },
ArrowRight: { x: OFFSET, y: 0 },
ArrowUp: { x: 0, y: -OFFSET },
ArrowDown: { x: 0, y: OFFSET },
};
Comment on lines +34 to +40
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's move these constants outside the callback, either to class property or outside the class, so we don't need to recreate them every keydown

const { key } = e;
const selectedEntity = this.manager.stateApi.getSelectedEntityAdapter();
const { x: offsetX = 0, y: offsetY = 0 } = offsets[key] || {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's clearer to do this:

const offsets = KEY_TO_OFFSETS[e.key]
if (!offsets) {
  return
}

// ...

selectedEntity.transformer.nudgePosition(offset.x, offset.y);


if (
!(selectedEntity && selectedEntity.$isInteractable.get() && $focusedRegion.get() === 'canvas') ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

Getting the selected entity is the most expensive part check, so we can be a bit more verbose to improve perf when checking if we should bail early. For example:

if ($focusedRegion.get() !== 'canvas') {
  return;
}
const offsets = KEY_TO_OFFSETS[e.key];
if (!offsets) {
  return;
}
const selectedEntity = this.manager.stateApi.getSelectedEntityAdapter();
if (!selectedEntity || !selectedEntity.$isInteractable.get()) {
  return;
}

Also, I forgot to mention we should also bail if this.manager.$isBusy.get() === true. This flag indicates that something else is happening on the canvas.

(offsetX === 0 && offsetY === 0)
) {
return; // Early return if no entity is selected or it is disabled or canvas is not focused
}

selectedEntity.transformer.nudgePosition(offsetX, offsetY);
};
}
12 changes: 10 additions & 2 deletions invokeai/frontend/web/src/features/controlLayers/konva/CanvasTool/CanvasToolModule.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,11 +515,19 @@ export class CanvasToolModule extends CanvasModuleBase {
};

onKeyDown = (e: KeyboardEvent) => {
if (e.repeat) {
if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) {
return;
}

if (e.target instanceof HTMLInputElement || e.target instanceof HTMLTextAreaElement) {
switch (
this.$tool.get() // before repeat, as we may want to catch repeating keys
) {
case 'move':
this.tools.move.onKeyDown(e);
break;
}

if (e.repeat) {
return;
}

Expand Down