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

Feature/wireframe #422

Merged
merged 14 commits into from
Dec 3, 2019
Merged

Feature/wireframe #422

merged 14 commits into from
Dec 3, 2019

Conversation

SridharJagannathan
Copy link
Collaborator

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)
  1. 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.
  2. Currently works for triangle meshes but the approach can be easily extended for quad meshes as well.

@coveralls
Copy link

coveralls commented Nov 26, 2019

Coverage Status

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

@SridharJagannathan
Copy link
Collaborator Author

SridharJagannathan commented Nov 26, 2019

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

nat/R/neuron-plot.R

Lines 65 to 66 in 3cf85f0

if (!add)
nclear3d(plotengine)

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
Copy link
Collaborator

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
Copy link
Collaborator Author

SridharJagannathan commented Nov 28, 2019

Things that still need to be done:

  • Support for quad mesh types
  • Support for argument add=TRUE
  • Test case for mesh properties
  • Test case for wire3d.shapelist3d

@SridharJagannathan
Copy link
Collaborator Author

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

R/wire3d.R Outdated
#' options(nat.plotengine = 'plotly')
#' wire3d(kcs20.mesh,alpha = 0.1, col = 'blue')
#' }
wire3d <- function(x, ..., add = FALSE, plotengine = getOption('nat.plotengine')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be add=TRUE to be consistent with current usage (where wire3d doesn't clear the plot)

@jefferis jefferis merged commit 0bffafb into master Dec 3, 2019
@jefferis
Copy link
Collaborator

jefferis commented Dec 3, 2019

Thank you!

@jefferis jefferis deleted the feature/wireframe branch December 3, 2019 12:05
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.

None yet

3 participants