Envelope input, canvas overlays, configurable cursor/fps, and controls hooks - #4
Merged
Conversation
…oints Four changes, all of the same kind: things a consumer was doing by reaching into phosphor's internals, which phosphor should own. Envelope input. SweepBuffer can now be fed (n_samples, n_channels, 2) -- an already-reduced (min, max) pair per sample -- instead of raw values. That is what a min/max decimator upstream produces, and accepting it here is what lets the reduction happen once near the source rather than shipping a full-rate signal across a process boundary only to throw most of it away. Column reduction takes the min of the mins and the max of the maxes, so the drawn result is identical to what the raw signal would have drawn, provided the upstream buckets are finer than a display column. There is a test asserting exactly that equivalence, since if it does not hold then moving decimation upstream changes what the user sees, and the whole point was that it should not. Note srate describes the stream being pushed, so in envelope mode it is the bucket rate. Sizing from the pre-decimation rate makes the ring `factor` times longer than the data filling it and the sweep sits mostly empty -- easy to get wrong, so it is documented and pinned by a test. Mismatched ranks now raise rather than being reinterpreted: a (n, ch, 2) array silently read as raw would plot 2*ch channels of interleaved bounds, which looks plausible and is wrong. DEFAULT_MAX_FPS was defined in constants.py and referenced nowhere. It is now the default for SweepConfig.max_fps, applied through a set_max_fps on ChannelPlotWidget so every plot type gets it. A caller wanting a render cap had to reach through _figure.canvas.set_update_mode itself. Cursor thickness and colour move into SweepConfig. phosphor derives the cursor width from the column gap, which is in time-world units and collapses to about a pixel; a consumer wanting a visible cursor was re-styling the fastplotlib graphic after every rebuild, on a timer, because the graphic is recreated on version bumps. The configured thickness is now a floor on the derived width. ChannelPlotControlsWidget gains add_controls() and insert_after(), backed by named slots recorded as the toolbar is built. The only way to extend this toolbar before was to scan the layout for a QLabel with matching text and count widgets past it -- which breaks on any relabel or reorder. insert_after raises on an unknown slot rather than dropping the widget, since "time" genuinely is conditional on the plot supporting time zoom. Adds a tests/ directory. SweepBuffer is pure numpy and threading, so the reduction logic -- where the risk in this change lives -- is testable without a canvas.
Per-trace identifiers and an amplitude calibration bar are base visual elements of a multichannel plot, not application furniture, so they belong next to the renderer rather than being rebuilt by every consumer. Both are plain QWidget painters -- Qt rather than GPU graphics because text is the point: crisp glyphs at arbitrary sizes, hinted and antialiased by the platform, with no vertex churn as the view scrolls. They are parented to the figure's Qt widget, the same trick the range label already used, and click-through so the plot keeps its wheel and hover events. The important part is who drives them. A consumer doing this outside phosphor has to poll: sync the geometry, re-measure the camera, and push the view every tick, because it cannot see the render loop. Here they update from _animation_callback, so they track scroll, paging and zoom by construction. The world->screen projection is measured from the real camera via map_world_to_screen rather than re-derived, and cached against the inputs that can change it, since it is a pygfx round-trip and the camera usually has not moved. When the probe fails the overlay falls back to reconstructing the layout analytically -- in a paint path, losing precise label placement beats losing the frame. set_scale_bar_text takes the label from the caller rather than formatting it. What a row-height *means* depends on the signal's units, which a plotting widget does not know; phosphor sizes the bar, the caller says what it represents. Labels are indexed by absolute channel and the overlay is told the scroll offset, so scrolling needs no reslicing by the caller. They are forwarded on identity change, not value: update_config swaps the list, and comparing 256 strings a frame to notice would cost more than the feature. Both overlays repaint only when something actually changed. Tests run on Qt's offscreen platform, so the geometry -- the part that is easy to get subtly wrong and hard to catch in a screenshot -- is covered headlessly. Includes the analytic fallback spanning the canvas, the degenerate zero-slope and single-channel cases, repaint gating, and that scrolling draws the right absolute labels.
Both from running the thing: with few channels visible, rows are hundreds of pixels tall and the label scaled with them, painting a large opaque chip over the signal it was labelling. Past ordinary reading size a bigger label conveys nothing, so it now stops at 14 px -- overridable per overlay for anyone who wants otherwise. Even capped, the chips cover trace on a dense plot, and once you know which channel is which you mostly want them gone. ChannelPlotWidget gains a channel_labels_visible property and the controls bar a "Labels" toggle, which is the first user of the extension points added a commit ago.
Labels usually arrive after the plot is built, since a source announces its channel names on its own schedule. Without a setter the only way to apply them was to assign _channel_labels and hope the overlay noticed -- which it would not, since it caches on identity.
Centred, it sat in the middle of the traces -- the part of the canvas with the most to look at. A calibration mark belongs in a corner, like a map's. Anchored at its foot rather than its centre, so the bottom edge stays put as the amplitude scale changes and the bar reads as a ruler standing on the canvas floor instead of something that drifts when you zoom.
The "A" key and the "Auto" button switched off the only thing that ever moves the camera. _init_rendering disables the pan/zoom controller, so turning autoscale off did not hand control to the user -- it stranded the view with no way to move it. On the sweep that showed up as a concrete bug. _time_zoom changes buf.display_dur and the axis label but never touches the camera; it relies on _apply_auto_scale running next frame. With autoscale off, zooming in halved the data's world extent while the camera stayed at the old width, so the trace filled the left half of a 5 s window and appeared to advance at half the rate the axis claimed. Fixing it rather than removing it would have meant making every zoom path drive the camera itself, to reach a state whose only distinguishing feature is that the view no longer follows the data. Nothing else uses it: SpectrumWidget and ScatterWidget do not override _apply_auto_scale and also run with no controller, so their off state is equally inert. _apply_auto_scale stays and now runs unconditionally -- it is not optional behaviour, it is how the camera gets framed at all -- with a docstring saying so. _zoom_amplitude's fallback path no longer disables autoscale as a side effect, which was the same trap in miniature. The controls slot after that group is renamed "autoscale" -> "labels", since the label toggle is what now sits there. Not affected: the evoked app's amplitude autoscale, which is a separate feature with its own implementation, and intent-tools references none of the removed symbols.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seven commits, all of the same kind: things a consumer was doing by reaching into phosphor's internals, which phosphor should own. Found while generalising a downstream viewer that had reimplemented all of them, and refined against a live 256-channel stream.
Envelope input
SweepBuffer(envelope=True)accepts(n_samples, n_channels, 2)— an already-reduced(min, max)pair per sample — instead of raw values. That is what a min/max decimator upstream produces, and accepting it is what lets the reduction happen once near the source rather than shipping a full-rate signal across a process boundary only to discard most of it.Column reduction takes min-of-mins and max-of-maxes, so the drawn result is identical to what the raw signal would have drawn, provided the upstream bins are finer than a display column. A test asserts exactly that equivalence — if it does not hold, moving decimation upstream changes what the user sees, which was the whole premise.
Measured downstream at 256 channels on a 5 s window: 15× less data across the boundary, and the frame rate roughly doubles at 2 ms bins. Coarser than about one display column (~2.5 ms there) and adjacent columns share a pair, which shows up as a visible zig-zag — worth knowing when choosing a bin size.
Two traps, both documented and tested:
sratedescribes the stream being pushed, so in envelope mode it is the bin rate. Sizing from the pre-decimation rate makes the ringfactortimes longer than the data filling it, and the sweep sits mostly empty.(n, ch, 2)array read as raw plots2*chchannels of interleaved bounds — plausible-looking and wrong.Canvas overlays
ChannelLabelOverlay(per-trace identifiers) andScaleBarOverlay(amplitude calibration bar) move in asphosphor.overlays. Base visual elements of a multichannel plot, not application furniture.Qt painters rather than GPU graphics because text is the point: crisp glyphs at arbitrary sizes, hinted by the platform, no vertex churn as the view scrolls.
The substantive part is who drives them. Outside phosphor a consumer has to poll — sync geometry, re-measure the camera, push the view every tick — because it cannot see the render loop. Here they update from
_animation_callback, so they track scroll, paging and zoom by construction. The world→screen projection is measured from the real camera viamap_world_to_screenand cached against the inputs that can change it; when the probe fails it falls back to reconstructing the layout analytically, because in a paint path losing precise label placement beats losing the frame.Label font is capped at 14 px and the labels are toggleable. Both from running it: at a low channel count rows are hundreds of pixels tall, and a label that scaled with them painted a large opaque chip over the signal it was naming. Even capped, the chips cover trace on a dense plot, so there is a "Labels" button.
The scale bar sits in the lower-right, anchored at its foot — centred it sat in the middle of the traces, and anchoring at the foot means the bottom edge stays put as the amplitude scale changes.
set_scale_bar_text()takes the label from the caller rather than formatting it: what a row-height means depends on the signal's units, which a plotting widget does not know. phosphor sizes the bar; the caller says what it represents.Autoscale toggle removed
The
Akey and theAutobutton switched off the only thing that ever moves the camera —_init_renderingdisables the pan/zoom controller, so turning autoscale off did not hand control to the user, it stranded the view.On the sweep that was a concrete bug:
_time_zoomchangesbuf.display_durand the axis label but never touches the camera, relying on_apply_auto_scalenext frame. With autoscale off, zooming in halved the data's extent while the camera stayed at the old width, so the trace filled the left half of the window and appeared to advance at half the rate the axis claimed.Nothing else used it —
SpectrumWidgetandScatterWidgetdo not override_apply_auto_scaleand also run with no controller, so their off state was equally inert._apply_auto_scalenow runs unconditionally, which is what it always effectively was._zoom_amplitude's fallback no longer disables autoscale as a side effect — the same trap in miniature, reachable without ever pressingA.Smaller API gaps
DEFAULT_MAX_FPSwas defined inconstants.pyand referenced nowhere. Now the default forSweepConfig.max_fps, applied throughChannelPlotWidget.set_max_fps()so every plot type gets it. A caller wanting a render cap had to reach through_figure.canvas.set_update_mode.SweepConfigfields. The geometry-derived width is in time-world units and collapses to about a pixel, so a consumer wanting a visible cursor was re-styling the fastplotlib graphic on a timer, because the graphic is recreated on version bumps.set_channel_labels()— labels usually arrive after the plot is built, and without a setter the only option was assigning_channel_labelsand hoping the overlay noticed, which it would not (it caches on identity).add_controls()/insert_after(slot, widget)onChannelPlotControlsWidget, backed by named slots. Extending this toolbar previously meant scanning the layout for aQLabelwith matching text and counting widgets past it.Tests
Adds
tests/— phosphor had none. 27 tests, all headless:SweepBufferis pure numpy, and the overlays run on Qt's offscreen platform, so the reduction and geometry logic is covered without a rendering backend.Not covered
The
SweepConfigplumbing,set_max_fps, and cursor styling are import-checked and lint-clean but not rendered — there is no GUI harness here and I did not add one. The envelope reduction and overlay geometry are well covered; the widget wiring around them is not. Everything has, however, been exercised by hand against a live 256-channel stream.Deliberately out of scope: exposing the amplitude scale in physical units, which needs the unit vocabulary (A/D counts, µV, dB, z-units) settled first.