dmurdoch / rgl

rgl is a 3D visualization system based on OpenGL. It provides a medium to high level interface for use in R, currently modelled on classic R graphics, with extensions to allow for interaction.
https://dmurdoch.github.io/rgl/
GNU General Public License v2.0
86 stars 21 forks source link

Partial argument names #170

Closed HenrikBengtsson closed 2 years ago

HenrikBengtsson commented 2 years ago

Hi Duncan,

I run R with options(warnPartialMatchArgs = TRUE) set and spotted some partial argument names in rgl. Specifically, rgl uses rep(<x>, len = <n>), which is short of rep(<x>, length.out = <n>) in:

$ grep -E "rep[(].*len *=" R/*.R
R/as.mesh3d.default.R:        cols <- rep(cols, len = ncol(mesh$vb))
R/as.mesh3d.default.R:        alpha <- rep(alpha, len = ncol(mesh$vb))
R/as.mesh3d.default.R:      newmat$color <- rep(newmat$color, len = n)
R/as.mesh3d.default.R:      newmat$alpha <- rep(newmat$alpha, len = n)
R/aspect3d.R:       x <- rep(x, len=3)
R/as.triangles3d.R:        attrib <- apply(attrib, 2, function(col) rep(col, len = nvert))
R/axes.R:  text <- rep(text, len = newlen)
R/axes.R:  line <- rep(line, len = newlen)
R/axes.R:    at <- rep(at, len = newlen)
R/cylinder3d.R:    e1 <- e1[rep(seq_len(nrow(e1)), len=n),] 
R/cylinder3d.R:    e2 <- e2[rep(seq_len(nrow(e2)), len=n),] 
R/cylinder3d.R:    e3 <- e3[rep(seq_len(nrow(e3)), len=n),] 
R/cylinder3d.R:  radius <- rep(radius, len=n)
R/cylinder3d.R:  twist <- rep(twist, len=n)
R/mesh3d.R:    x <- rep(normals$x, len = nvertex)
R/mesh3d.R:    y <- rep(normals$y, len = nvertex)
R/mesh3d.R:    z <- rep(normals$z, len = nvertex)
R/mesh3d.R:    x <- rep(texcoords$x, len = nvertex)
R/mesh3d.R:    y <- rep(texcoords$y, len = nvertex)
R/scene.R:      x <- rep(normals$x, len=nvertex)
R/scene.R:      y <- rep(normals$y, len=nvertex)
R/scene.R:      z <- rep(normals$z, len=nvertex)
R/scene.R:      s <- rep(texcoords$x, len=nvertex)
R/scene.R:      t <- rep(texcoords$y, len=nvertex)
R/scene.R:  family <- rep(family, len=nfonts)
R/scene.R:  font <- rep(font, len=nfonts)
R/scene.R:  cex <- rep(cex, len=nfonts)  

This is with:

$ git log -1
commit 232dfab47615baeb3fce3b4913b8b2f435120520 (HEAD -> master, origin/planebug, origin/master, origin/HEAD)
Author: Duncan Murdoch <murdoch.duncan@gmail.com>
Date:   Mon Jan 17 04:50:05 2022 -0500

    Add link to Stéphane Laurent's blog post.
HenrikBengtsson commented 2 years ago

There's also a few seq(..., len = <n>) and seq(..., length = <n>) that should be seq(..., length.out = <n>):

$ grep -E "seq[(].*len *=" R/*.R
R/arrow3d.R: phi <- seq(pi/nbarbs, 2*pi-pi/nbarbs, len = nbarbs)
R/cylinder3d.R:    theta <- seq(0, 2*pi, len=sides+1)[-1]
R/persp3d.R:function(x = seq(0, 1, len = nrow(z)), y = seq(0, 1, len = ncol(z)),
R/persp3d.R:                x <- seq(0, 1, len = nrow(z))
R/turn3d.R:  theta <- seq(0, 2*pi, len = n + 1)[-(n + 1)]

$ grep -E "seq[(].*length *=" R/*.R
R/material.R:       alpha = if (idata[11]) ddata[seq(from=6, length=idata[11])] else 1,
dmurdoch commented 2 years ago

Thanks for pointing those out. There's also at least one more: bg3d() sets fog=TRUE, intending it to be treated as a material property, but it partially matches the fogScale argument in rgl.bg. So that was a real bug, which I'll fix at the same time as the others.

HenrikBengtsson commented 2 years ago

Thanks for this.

Thanks for pointing those out. There's also at least one more: bg3d() sets fog=TRUE, intending it to be treated as a material property, but it partially matches the fogScale argument in rgl.bg. So that was a real bug, which I'll fix at the same time as the others.

I did indeed also see a partial argument warning about fog = TRUE when running some rayshader examples, but I got distracted and decided to postpone tracking down the exact source. I would have thought it was short for fogScale. I'm glad you caught a real bug this way. This is exactly why I run with options(warnPartialMatchArgs = TRUE) and report upstream whenever I see something. In 99.99% of the cases, they're innocent warnings, but once in a while, there's something real. I wish there was an options(errPartialMatchArgs = TRUE), especially something that could be enabled by R CMD check.