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

Add settings to store initial scene position #1686

Closed
wants to merge 4 commits into from
Closed

Add settings to store initial scene position #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5eaae73
add settings to store initial scene position
gautamv95 Apr 23, 2020
8a85918
changed variable names
gautamv95 Apr 24, 2020
d990c83
store settings for space + drag/drop and add docs
gautamv95 Apr 30, 2020
caed671
store settings for touch devices and modified docs
gautamv95 May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion Core/GDCore/IDE/Dialogs/LayoutEditorCanvas/LayoutEditorCanvasOptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ LayoutEditorCanvasOptions::LayoutEditorCanvasOptions()
gridG(180),
gridB(255),
zoomFactor(1),
windowMask(false) {}
windowMask(false),
viewXPosition(0),
viewYPosition(0),
viewPositionValid(false) {}

void LayoutEditorCanvasOptions::SerializeTo(SerializerElement& element) const {
element.SetAttribute("grid", grid);
Expand All @@ -35,20 +38,26 @@ void LayoutEditorCanvasOptions::SerializeTo(SerializerElement& element) const {
element.SetAttribute("gridB", gridB);
element.SetAttribute("zoomFactor", zoomFactor);
element.SetAttribute("windowMask", windowMask);
element.SetAttribute("viewXPosition", viewXPosition);
element.SetAttribute("viewYPosition", viewYPosition);
element.SetAttribute("viewPositionValid", viewPositionValid);
}

void LayoutEditorCanvasOptions::UnserializeFrom(
const SerializerElement& element) {
grid = element.GetBoolAttribute("grid");
snap = element.GetBoolAttribute("snap");
windowMask = element.GetBoolAttribute("windowMask");
viewPositionValid = element.GetBoolAttribute("viewPositionValid", false);
gridWidth = element.GetIntAttribute("gridWidth", 32);
gridHeight = element.GetIntAttribute("gridHeight", 32);
gridOffsetX = element.GetIntAttribute("gridOffsetX", 0);
gridOffsetY = element.GetIntAttribute("gridOffsetY", 0);
gridR = element.GetIntAttribute("gridR", 158);
gridG = element.GetIntAttribute("gridG", 180);
gridB = element.GetIntAttribute("gridB", 255);
viewXPosition = element.GetIntAttribute("viewXPosition", 0);
viewYPosition = element.GetIntAttribute("viewYPosition", 0);
zoomFactor = element.GetDoubleAttribute("zoomFactor", 1.0);
}

Expand Down
3 changes: 3 additions & 0 deletions Core/GDCore/IDE/Dialogs/LayoutEditorCanvas/LayoutEditorCanvasOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class GD_CORE_API LayoutEditorCanvasOptions {
int gridB; ///< Grid blue color in editor
float zoomFactor; ///< Stores the zoom factor
bool windowMask; ///< True if window mask displayed in editor
int viewXPosition; ///< X coordinate of the view displayed by the editor
int viewYPosition; ///< Y coordinate of the view displayed by the editor
bool viewPositionValid; ///< If false, the viewXPosition and viewYPosition are not set and does not represent the default view position
};

} // namespace gd
Expand Down
35 changes: 33 additions & 2 deletions newIDE/app/src/InstancesEditor/FullSizeInstancesEditorWithScrollbars.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,15 @@ export default class FullSizeInstancesEditorWithScrollbars extends Component<
};

componentDidMount() {
if (this._editor) {
this._handleViewPositionChange(this._editor.getViewPosition());
const editor = this._editor;
if (editor) {
this._handleViewPositionChange(editor.getViewPosition());
if (this.props.options.viewPositionValid) {
editor.scrollTo(
this.props.options.viewXPosition,
this.props.options.viewYPosition
);
}
}
}

Expand All @@ -78,6 +85,15 @@ export default class FullSizeInstancesEditorWithScrollbars extends Component<
}
}
);

//Prevent reduntant calls to onChangeOptions
if (this.props.options.viewXPosition !== xValue) {
this.props.onChangeOptions({
viewXPosition: this.state.xValue,
viewYPosition: this.state.yValue,
viewPositionValid: true,
gautamv95 marked this conversation as resolved.
Show resolved Hide resolved
});
}
};

_handleYChange = (e: any, value: number) => {
Expand All @@ -95,6 +111,15 @@ export default class FullSizeInstancesEditorWithScrollbars extends Component<
}
}
);

//Prevent reduntant calls to onChangeOptions
if (this.props.options.viewYPosition !== yValue) {
this.props.onChangeOptions({
viewXPosition: this.state.xValue,
viewYPosition: this.state.yValue,
viewPositionValid: true,
gautamv95 marked this conversation as resolved.
Show resolved Hide resolved
});
}
};

_setAndAdjust = ({ xValue, yValue }: { xValue: number, yValue: number }) => {
Expand All @@ -109,6 +134,12 @@ export default class FullSizeInstancesEditorWithScrollbars extends Component<
xValue,
yValue,
});

this.props.onChangeOptions({
viewXPosition: this.state.xValue,
viewYPosition: this.state.yValue,
viewPositionValid: true,
});
};

_handleViewPositionChange = throttle(
Expand Down
12 changes: 11 additions & 1 deletion newIDE/app/src/InstancesEditor/PinchHandler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type Props = {|
setZoomFactor: number => void,
getZoomFactor: () => number,
viewPosition: ViewPosition,
setViewPosition: ViewPosition => void,
|};

/**
Expand All @@ -108,12 +109,20 @@ export default class PinchHandler {
_setZoomFactor: number => void;
_getZoomFactor: () => number;
_viewPosition: ViewPosition;
_setViewPosition: ViewPosition => void;
_unregisterCanvasPinchDetector: () => void;

constructor({ canvas, setZoomFactor, getZoomFactor, viewPosition }: Props) {
constructor({
canvas,
setZoomFactor,
getZoomFactor,
viewPosition,
setViewPosition,
}: Props) {
this._setZoomFactor = setZoomFactor;
this._getZoomFactor = getZoomFactor;
this._viewPosition = viewPosition;
this._setViewPosition = setViewPosition;
this._unregisterCanvasPinchDetector = registerCanvasPinchDetector({
canvas,
onPinchStart: this._startPinch,
Expand Down Expand Up @@ -142,6 +151,7 @@ export default class PinchHandler {
this._lastPinchCenterY = centerY;

this._setZoomFactor(this._getZoomFactor() * scale);
this._setViewPosition(this._viewPosition);
Copy link
Owner

Choose a reason for hiding this comment

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

I think this function is not properly named, and doing too much.

Think of how the class is used:
The parent, InstancesEditor, must create PinchHandler by passing it a viewPosition. This means that InstancesEditor knows, and actually owns viewPosition.
Now it must also pass it something called "setViewPosition" used to... set a new ViewPosition? But why it would need to do that, because InstancesEditor is actually the owner of ViewPosition.

You probably did this because you observed that onViewPositionChanged is passing the ViewPosition as argument. This is right... but onViewPositionChanged is a prop of InstancesEditor! This means that it needs to pass ViewPosition because the parent of InstancesEditor does not have access to ViewPosition - because it's owned by InstancesEditor.

Here, it does not really make sense to pass to PinchHandler a function that takes a ViewPosition. Think like the parent (InstancesEditor): "why are you passing my a ViewPosition? I know ViewPosition, it's mine! It's me that passed it to you, why are you giving it back in your callback? I don't even need it, leave me in peace!" ;)
Or think like the child (PinchHandler): "why are you asking me for the ViewPosition? It's you that passed it to me!".

Instead, what you want is to get notified when the view is moved. And PinchHandler is moving the position, by calling scrollBy.
The solution:

  • pass a function called onViewPositionChanged to PinchHandler, which is called (as the name implies) "when the view position is changed". But without argument: _onViewPositionChanged: () => void .
    Then it's the responsibility of the parent to react accordingly, providing more info if needed to it's parent. In other words, in InstancesEditor, you do: 
    onViewPositionChanged: () => { this.props.onViewPositionChanged(this.viewPosition) }

Actually there is maybe a better solution. Why is InstancesEditor having to call manually onViewPositionChanged, and now why is PinchHandler having to call its own onViewPositionChanged every time we call scrollBy (and scrollToInstance)?

It's manual and error-prone, we could forget it in the future... instead why not give this responsibility to... ViewPosition.

    this.viewPosition = new ViewPosition({
      initialViewX: project ? project.getGameResolutionWidth() / 2 : 0,
      initialViewY: project ? project.getGameResolutionHeight() / 2 : 0,
      width: this.props.width,
      height: this.props.height,
      options: this.props.options,
      onViewPositionChanged: () => { this.props.onViewPositionChanged(this.viewPosition); }
    });

and then you can call onViewPositionChanged inside scrollBy and scrollToInstance.
And you remove all the other calls to this.props.onViewPositionChanged - they are now redundant because ViewPosition is doing the job of notifying after any change.

Hope this make sense. :)

};

_endPinch = () => {
Expand Down
1 change: 1 addition & 0 deletions newIDE/app/src/InstancesEditor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ export default class InstancesEditor extends Component<Props> {
setZoomFactor: this.setZoomFactor,
getZoomFactor: this.getZoomFactor,
viewPosition: this.viewPosition,
setViewPosition: this.props.onViewPositionChanged,
});

this.canvasCursor = new CanvasCursor({
Expand Down