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

Custom frame axes options: scale, dashed lines and no pan&zoom frame #208

Merged

Conversation

viktorku
Copy link
Member

Adds an ability to optionally specify:

  • Custom Axes and OrbitControl's "temporary" pan and zoom frame line type to "dashed" and custom scale, as it would be nice if we display some frames with a different scale/line-type (besides the thicknes - which we can already do customize).
  • Also the "temporary" pan and zoom frame can be omitted from being drawn, as sometimes it might just unnecessary clutter the view.

Backwards compatibility with default values is being retained, but please confirm.

image

Credits go to @sdeleersnijder ;)

@viktorku viktorku changed the title Dashed frame axes option Custom frame axes options: scale, dashed lines and no pan&zoom frame Jan 15, 2018
line.position.copy(axis);
// Make spacing between dashes equal to 1.5 times the dash length.
line.position.multiplyScalar(l / 2 + 3 * l * i);
line.quaternion.copy(rot);
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a few duplicated lines here - can you refactor the code to move them outside the if-else?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are not duplicated, but inside a for loop. The de-duplication would make reasoning about this harder.

@@ -61,17 +69,19 @@ ROS3D.OrbitControls = function(options) {
};
var state = STATE.NONE;

// add the axes for the main coordinate frame
this.axes = new ROS3D.Axes({
Copy link
Member

Choose a reason for hiding this comment

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

this.axes isn't used if displayPanAndZoomFrame is false. Could you move this under if statement? And update showAxes() something like

 if(this.displayPanAndZoomFrame){
 var that = this;                                                                                                                                                                                                                                                           
  this.axes.traverse(function(obj) {                                                                                                   
    obj.visible = true;                                                                                                                
  });                                                                                                                                  
  if (this.hideTimeout) {                                                                                                              
    clearTimeout(this.hideTimeout);                                                                                                    
  }                                                                                                                                    
  this.hideTimeout = setTimeout(function() {                                                                                           
    that.axes.traverse(function(obj) {                                                                                                 
      obj.visible = false;                                                                                                             
    });                                                                                                                                
    that.hideTimeout = false;                                                                                                          
  }, 1000);                                                                                                                            
}

Copy link
Member

Choose a reason for hiding this comment

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

@jihoonl
Copy link
Member

jihoonl commented Apr 9, 2018

It is an interesting feature that I would like to merge in. @viktorku any updates?

@keego keego mentioned this pull request Sep 6, 2018
6 tasks
@jihoonl jihoonl merged commit af04c98 into RobotWebTools:develop Feb 2, 2019
@mvollrath mvollrath mentioned this pull request Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants