Skip to content

gcurve/gdots constructor ignores size= (writes a dead _size; the derived property reports the default) #287

Description

@sspickle

gcurve(size=…) / gdots(size=…) is silently ignored: the constructed object keeps the default radius and the browser is sent the default size of 6, whatever the caller asked for. Setting .size after construction works, and so does radius= in the constructor — only the constructor's size= is lost.

from vpython import *
g = graph()
d = gdots(graph=g, size=8, color=color.blue)
print(d.size)      # 6   — expected 8
d.size = 8
print(d.size)      # 8   — the setter is fine

Cause

gobj.size is a derived property backed by _radius (vpython/vpython.py:2207-2212):

@property
def size(self): return 2*self._radius
@size.setter
def size(self, val):
    self._radius = val/2
    self.addattr('radius')

But gobj.setup's scalar-argument loop (vpython/vpython.py:2168-2174) writes the private name directly, bypassing the property:

for a, val in args.items():
    argsToSend.append(a)
    if a == 'graph':
        val = val.idx
    setattr(self, '_' + a, val)          # size=8  ->  self._size = 8

self._size is a dead attribute — nothing in the package reads it. The send loop immediately below then reads back through the property:

for a in argsToSend:
    ...
    aval = getattr(self, a)              # getattr(self,'size') -> 2*self._radius -> 6
    cmd[a] = aval

so size: 6 goes on the wire and _size is never consulted again. Minimal demonstration:

class Fake:
    def __init__(self): self._radius = 3
    @property
    def size(self): return 2*self._radius
    @size.setter
    def size(self, val): self._radius = val/2

o = Fake()
setattr(o, '_size', 8)        # what gobj.setup does
o._size                       # 8   (dead)
getattr(o, 'size')            # 6   <-- what is sent

radius=8 works for the same reason in reverse: _radius is the real backing attribute, so setattr(self, '_radius', 8) lands in the right slot and getattr(self, 'radius') reads it back.

Scope

I swept every @property in the gobj class body (lines 2122-2338) for getters that are not simply return self._<name>. size is the only one, so this looks like a single-attribute bug rather than a pattern — but the mechanism (setattr(self, '_' + a, val) against a derived property) would bite any future attribute defined the same way, which is the part worth guarding.

Not verified beyond gobj: other classes have their own setup methods with the same setattr(self, '_' + a, val) idiom and their own derived size/axis properties (e.g. vpython.py:815, :1205, :1322, :1623), where size and axis interact. Those may be fine — the interaction is handled explicitly there — but they are the same shape.

Suggested fix

In gobj.setup's scalar loop, route through the property when one exists, or special-case the derived name:

for a, val in args.items():
    argsToSend.append(a)
    if a == 'graph':
        val = val.idx
    if a == 'size':                       # derived: backed by _radius, not _size
        self._radius = val/2
    else:
        setattr(self, '_' + a, val)

Setting _radius directly rather than calling the public setter is deliberate: the size setter also calls addattr('radius'), and addattr spins on baseObj.sent (vpython.py:305-308). During construction that is at best redundant — the value is already going out in this constructor's own cmd — and on a single-threaded host (Pyodide/wasm) a spin-wait inside a constructor is a deadlock rather than a delay.

Happy to open a PR. Two questions if you have a preference:

  1. Special-case size, or make the loop property-aware in general (if isinstance(getattr(type(self), a, None), property))? The general form guards the mechanism; the special case is smaller and cannot surprise anything else.
  2. Should the other setup implementations with derived size/axis get the same audit in the same change, or separately?

How it surfaced

Found while adopting this package to run Web VPython under Pyodide in a Web Worker for trinket. Trinket's existing bridge bypasses gobj.setup entirely (it forwards constructor kwargs straight to the GlowScript object), so gdots(size=8) works there today and stops working when the real package is used — which is how a long-standing bug turned up as an apparent regression. Nothing about the worker or the transport is involved; the wire package carries size: 6 on the notebook path too.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions