natverse / nat

NeuroAnatomy Toolbox: An R package for the (3D) visualisation and analysis of biological image data, especially tracings of single neurons.
https://natverse.org/nat/
64 stars 29 forks source link

Feature/wireframe #422

Closed SridharJagannathan closed 5 years ago

SridharJagannathan commented 5 years ago

Updated for #421 @jefferis:

  1. Along with this implementation I have also fixed a bug in commit 3cf85f0 which can be reproduced as follows:
    nclear3d()
    plot3d(Cell07PNs[[2]],col='blue',add=FALSE)
  2. For the wireframe implementation, I have made a new function wire3d that masks the function with the same name from rgl and will pass the call to either rgl or plotly dependent on the option of nat.plotengine.
  3. Currently works for triangle meshes but the approach can be easily extended for quad meshes as well.
coveralls commented 5 years ago

Coverage Status

Coverage increased (+0.1%) to 83.45% when pulling 8933f6dc298fcfb375d076adaa8ada27792b7f21 on feature/wireframe into 6c7f84485191e2c9da24a14c07c1d5a1e4ce3fa2 on master.

SridharJagannathan commented 5 years ago

Hi @SridharJagannathan, I see the problem, but that's not the solution. The bug is here:

https://github.com/natverse/nat/blob/3cf85f0bb370801287ff7f37f1b714b0270d77a4/R/neuron-plot.R#L65-L66

That should have read:

nclear3d(plotengine=plotengine)

@jefferis Thanks for this, I see your point, isn't it usually the convention that when a function has one formal argument, followed by ellipsis, people usually can supply that argument without naming them, or to frame it better which is usually preferred, nclear3d <- function(plotengine = getOption('nat.plotengine'),...) where people can simply use nclear3d(plotengine) or nclear3d <- function(...,plotengine = getOption('nat.plotengine'))

jefferis commented 5 years ago

It's because nclear3d is actually a wrapper around rgl::clear3d and most of the time you don't need to touch the plotengine argument because the default value works fine. You see examples of ellipsis first where the subsequent arguments are options rather than the main deal. c() is an example of this.

SridharJagannathan commented 5 years ago

Things that still need to be done:

SridharJagannathan commented 5 years ago

@jefferis: I have updated all the comments per our discussion, please review this and merge the same.

jefferis commented 5 years ago

Thank you!