Add the picogame game engine module - #11199
Conversation
Retained-mode 2D engine for writing games in CircuitPython: a Scene with dirty-rect rendering over Sprite/Tilemap/Canvas/StripDraw/ Particles/Triangles layers, plus collision, noise and text helpers - and enough pseudo-3D primitives (project, raycast, mode-7, triangle batches) for simple 3D games. Renders portably through any BusDisplay; optional port backends add an async-DMA SPI path (raspberrypi, espressif) and a RAM-framebuffer target for DVI/HSTX scanout boards. Gated by CIRCUITPY_PICOGAME (off by default); enabled on pajenicko_picopad and adafruit_fruit_jam.
tannewt
left a comment
There was a problem hiding this comment.
Thanks for the PR! I've got a few organizational comments but most of the self-contained stuff is totally fine as-is.
Review feedback. The types that were consolidated in __init__.c (Bitmap, Sprite, StripDraw, Triangles, Framebuffer) each get their own shared-bindings file, matching Canvas/Scene/Tilemap/Particles/Display, so __init__.c is the module level only: its docstring, the module functions and the globals table. The docstring now also says how picogame relates to displayio - same display object, different way to drive it, no retained pixels - which is the first thing a reader of that file should learn. The eight error messages the module added are replaced with ones CircuitPython already ships (mp_arg_validate_type / mp_arg_error_invalid / mp_arg_validate_length_min, and m_malloc_fail for the unreachable scene-cap guard): the module now contributes no new strings to the translations.
tannewt
left a comment
There was a problem hiding this comment.
Thanks for splitting these apart. Many of the primitives are giving me "displayio" vibes so I think we could work to integrate more of this in the long term. Totally fine to have it separate now.
| #if CIRCUITPY_PICOGAME_FAST_DISPLAY | ||
| { MP_ROM_QSTR(MP_QSTR_Display), MP_ROM_PTR(&picogame_display_type) }, | ||
| #endif |
There was a problem hiding this comment.
Generally I don't like attributes only added in some cases. The way we do this is by raising NotImplementedError from the shared-module/common-hal implementation to give a clearer error message. I think it'd be good to do here too.
There was a problem hiding this comment.
I replaced it with two new constants FAST_DISPLAY_SUPPORTED and FRAMEBUFFER_SUPPORTED for features detection (it is used in picogame libs).
The file header still said the types were consolidated here, which stopped being true when they moved into their own files, and the displayio paragraph now names the shared object instead of calling it "the seam". Splitting Sprite out also left `bitmap` documented twice - once in the attribute summary and again at its property - which mypy rejects, so `make check-stubs` (the docs job) failed. The summary entry is gone; the fuller description at the property stays.
Replace the combined getter/setter with two methods, per review. get_tile(tx, ty) is a plain 3-arg binding (no kwarg parsing on the read path); set_tile(tx, ty, value) keeps the flip_x/flip_y/transpose orientation keywords. Out-of-range behavior is unchanged: reads return 0, writes are ignored.
Builds without the corresponding backend get a stub type whose constructor raises NotImplementedError (the same core message the unsupported rgb444= path uses), so the module's attributes no longer vary with build flags. Adds FAST_DISPLAY_SUPPORTED and FRAMEBUFFER_SUPPORTED alongside RGB444_SUPPORTED for feature detection. +216 B on RP2040.
Two measured optimizations from device profiling: - fill_triangles: route the edge-slope divides through a 32-bit path when the deltas fit in 16 bits (they almost always do); the 64-bit soft-divide dominated the rasterizer on M33. - blit_bitmap_scaled: opaque PAL8 sprites at power-of-two scales (2x/4x/8x) skip the generic DDA scaler - one palette lookup per source pixel, repeated rows copied. Measured 1.5x on a full scaled blit (RP2350), byte-identical to the generic path across 200k randomized windows. Powers of two only: at other scales the generic step rounding samples differently, so those keep the existing path.
tannewt
left a comment
There was a problem hiding this comment.
Getting close! I added a couple comments. The main thing is taking a look at the generated API docs. You'll want to make sure each function gets documented correctly. It not only supports the API docs but also the stubs for completion and validation.
Reviewed against docs/design_guide.rst and the shared-bindings corpus: - parameters move from prose to :param: field lists; buffer APIs state element types, layouts and minimum sizes - every function, constructor and drawing primitive gets a docstring, and the six exported constants (STRIP_H, FPU, API_LEVEL and the three capability flags) get stubs - documented signatures match the runtime: Scene border insets and the target/layer unions, StripDraw always_dirty, float sprite positions - grouped attribute docstrings split so every attribute renders - factual fixes: framebuffer inversion happens during composition, always_dirty=False still renders once initially, near() is strictly less-than, Framebuffer output options are mutually exclusive - external helpers linked once from the module docstring instead of bare names throughout; measured numbers and rationale removed; ALL-CAPS emphasis and British spellings normalized; a module-level example added
Module-level render() now takes its layer list as `layers` - it accepts every layer kind, and the documentation already described it that way; `sprites` remains the keyword on Display.render(), which really is sprites-only. Nothing in the shipped ecosystem passed it by keyword. StripDraw.invalidate() with a partial rectangle now raises ValueError instead of silently repainting the whole layer: the only meaningful forms are no rectangle or all four values, and a partial one is a bug in the caller. Scene's strip buffers become optional, defaulting to None: a Framebuffer-target scene never uses them, and requiring two explicit None arguments was a wart every framebuffer board would carry forever. Buffer-using targets still validate them at construction. Their documentation now also states that the buffer size selects the strip height rather than merely satisfying a minimum.
From two independent editorial reviews of the rendered page:
- the low-level compute functions (project, raycast, road_edges) move
after the everyday API in the page order instead of greeting the
reader right below the module intro
- doubled words removed ("frame frame"), the last "wire byte order"
unified to transfer byte order, "arbitrary size" phrasing fixed,
transpose no longer described as rotation
- Bitmap.stride documents its 0-means-default sentinel; project
explains why the right vector has no y component; fbm1d is described
on its own terms
- the ASCII limitation moves to Canvas.text itself; StripDraw's
stale-area note moves to the class docstring where all four
rectangle attributes are in scope; always_dirty is described inline
instead of a circular reference
- per-function helper mentions shortened now that the module note
covers distribution
Scene, render() and invert() resolved a Framebuffer with a bare type check, so an instance of a Python subclass fell through to the busdisplay branch and the renderer cast the wrong pointer - a memory fault instead of an error. BusDisplay targets already accepted subclasses through mp_obj_cast_to_native_base; the Framebuffer and Display checks now do the same and resolve to the native base object. Found by running the module example against a Framebuffer subclass.
tannewt
left a comment
There was a problem hiding this comment.
Looks great! Thanks for the doc review and the awesome module.
First step of #11198: the picogame core C module.
The module itself is self-contained (
shared-bindings/picogame,shared-module/picogame, optional port backends incommon-hal/picogame). Outside of it this PR only touches:pajenicko_picopad- enables the engine. To fit it next to the Wi-Fi stack the board drops peripherals it physically lacks (_EVE,qrio,picodvi) and switches from -O3 to -O2 plus a few measured loop flags (engine kernels within ±1 % of -O3, ~150 KB smaller). 87.9 % of the firmware region used.adafruit_fruit_jam- enables the engine with the RAM-framebuffer target (DVI/HSTX scanout). 92.2 % used.locale/circuitpython.pot- regenerated.Left out for follow-up PRs (per the issue discussion): core1 rendering and the ROMFS asset region.
Both board builds, sphinx docs and translations pass locally.