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.

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

Virtual-Tour: Rotate before and after a new node, chain nodes #933

Closed
wants to merge 6 commits into from
Closed

Virtual-Tour: Rotate before and after a new node, chain nodes #933

wants to merge 6 commits into from

Conversation

Mr-Kanister
Copy link

For my usecase I added the following features to the Virtual-Tour-Plugin:

  • it is possible to overwrite the position to which the camera rotates to, when a link is clicked. This is useful when two links are too close to each other and thus the distance between them (to prevent them from overlapping) needs to be increased. To further ensure that the camera still animates to the right place, this option has been added.
  • it is possible to specify a position to which the viewer rotates to, after the new node has been loaded. This is useful when you want to highlight specific things at a node in the tour.
  • it is possible to automatically jump to another node after the first one has loaded. With this option it is possible to create "chains" of nodes, which is especially useful when a place is not of great importance in the tour, but it is helpful for orientation to include it as an intermediate step (e.g. on a stair landing, or in an anteroom).

I'd be very happy, if these would make it upstream. Please tell me, what you think about the implementation.

Merge request checklist

  • All lints and tests pass.
  • If needed, the documentation has been updated. (Not yet)

Copy link
Owner

@mistic100 mistic100 left a comment

Choose a reason for hiding this comment

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

  • rotateBeforeLoad only works with position, when using gps the user won't know what is the final computed position. It would be better to have an offsetPosition which will be added to the link position (hence moving the marker/arrow but not where the viewer is rotated before going to the next node)

  • rotateAfterLoad and nextNodeId are advanced features that can be easily implemented with event listeners, it is be much more versatile

(the following code changes requests won't be needed once the above modifications are made)

@@ -342,6 +342,7 @@ export class VirtualTourPlugin extends AbstractConfigurablePlugin<

const fromNode = this.state.currentNode;
const fromLinkPosition = fromNode && fromLink ? this.__getLinkPosition(fromNode, fromLink) : null;
const rotateBeforeLoad = fromNode && fromLink && fromLink.rotateBeforeLoad ? fromLink.rotateBeforeLoad : fromLinkPosition;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const rotateBeforeLoad = fromNode && fromLink && fromLink.rotateBeforeLoad ? fromLink.rotateBeforeLoad : fromLinkPosition;
const rotateBeforeLoad = fromLink?.rotateBeforeLoad ?? fromLinkPosition;

@@ -432,6 +433,24 @@ export class VirtualTourPlugin extends AbstractConfigurablePlugin<

return true;
})
.then(() => {
const currentNode = this.state.currentNode;
const rotateAfterLoad = fromNode && fromLink && fromLink.rotateAfterLoad ? fromLink.rotateAfterLoad : null;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const rotateAfterLoad = fromNode && fromLink && fromLink.rotateAfterLoad ? fromLink.rotateAfterLoad : null;
const rotateAfterLoad = fromLink?.rotateAfterLoad;

@@ -432,6 +433,24 @@ export class VirtualTourPlugin extends AbstractConfigurablePlugin<

return true;
})
.then(() => {
Copy link
Owner

Choose a reason for hiding this comment

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

there is not point of chaining a promise here, the code can be put on line 433

}
})
.then(() => {
const nextNodeId = fromNode && fromLink ? fromLink.nextNodeId : null;
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
const nextNodeId = fromNode && fromLink ? fromLink.nextNodeId : null;
const nextNodeId = fromLink?.nextNodeId;

@Mr-Kanister
Copy link
Author

rotateAfterLoad and nextNodeId are advanced features that can be easily implemented with event listeners, it is be much more versatile

Do you mean those features shouldn't be implemented, or should they be implemented with a NodeChanged event listener?

@mistic100
Copy link
Owner

Do you mean those features shouldn't be implemented, or should they be implemented with a NodeChanged event listener?

They shouldn't be implemented

@Mr-Kanister
Copy link
Author

I can't really understand why the build fails, I implemented your requests.

@mistic100
Copy link
Owner

don't worry that's on me, it is broken on the main branch too

@mistic100 mistic100 added this to the 5.1.5 milestone Apr 21, 2023
@mistic100 mistic100 closed this in d01a892 Apr 21, 2023
@github-actions
Copy link

This feature/bug fix has been released in version 5.1.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants