Repository navigation
gcurve/gdots constructor ignores size= (writes a dead _size; the derived property reports the default) #287
Description
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 fineCause
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 = 8self._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] = avalso 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 sentradius=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:
- 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. - Should the other
setupimplementations with derivedsize/axisget 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.
Activity
fix(graph): gcurve/gdots constructor honours size= (#287)
Merge pull request #290 from vpython/fix/gobj-size-derived