Repository navigation
Tests: require_component instead of Optional get_component (review fix from #23) - #27
Conversation
Address review on #23: store the component add_component() returns as self.rigidbody instead of looking it up per access, use the Surface from load_image() directly in Tile, switch test_cloud to cloud.rigidbody, and put the clamp01 import in order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| renderer = self.add_component(SpriteRenderer2D) | ||
| renderer.set_image(load_image(ImagePath("tile"))) | ||
| image = renderer.require_image() | ||
| image = load_image(ImagePath("tile")) |
There was a problem hiding this comment.
Review: load_image(ImagePath("tile")) with no size returns the shared cached surface from pygamine's _cache, not a copy. Line 47 then runs polygon(self.unselected_surface, ...) on it, so the grey grid outline gets drawn into the global cache, once for every tile (64+ times on the same pixels). The bug was already there, but this rewrite makes image the cached object directly. Anything else that loads "tile" later (another map size, a UI preview) gets the outline baked in. Fix: image = load_image(ImagePath("tile")).copy().
| image_path=self.get_image_path(), | ||
| ) | ||
| self.add_component(Rigidbody2D).set_velocity((0, 0)) | ||
| self.rigidbody = self.add_component(Rigidbody2D) |
There was a problem hiding this comment.
Review: self.rigidbody is now a second reference next to _components["Rigidbody2D"]. add_component overwrites the entry by class name, so if a Rigidbody2D is ever re-added or replaced, the engine would update the new component while rigidbody (and so is_moving, has_arrived, control_moving) keeps reading the old one, which stays at the old velocity. The removed require_component property always returned the live component. Low risk today, since nothing re-adds it, but it is state that can drift. If the only goal was the Optional in tests/test_cloud.py, require_component(Rigidbody2D) in the test fixes that without the alias.
| companion.require_component(Rigidbody2D).set_velocity( | ||
| cloud.require_component(Rigidbody2D).velocity | ||
| ) | ||
| companion.rigidbody.set_velocity(cloud.rigidbody.velocity) |
There was a problem hiding this comment.
Review: GameObjectList subclasses a plain list, so cloud here and in update() (for cloud in self) is Any. That means cloud.rigidbody / companion.rigidbody are not type-checked: a typo or a rename passes mypy, and the "typed attribute" gives no benefit in this file. Consider annotating the container (e.g. make CloudContainer a list[Cloud] or annotate the loop variable) so the attribute is actually typed where it's used most.
| assert 0 <= cloud.rect.x <= SURFACE_SIZE[0] - 101 | ||
| assert 0 <= cloud.rect.y <= SURFACE_SIZE[1] - 101 | ||
| rb = cloud.get_component(Rigidbody2D) | ||
| rb = cloud.rigidbody |
There was a problem hiding this comment.
Review: the tests now set and read velocity only through the cloud.rigidbody alias. Nothing checks that the alias is the component the engine updates (cloud.rigidbody is cloud.get_component(Rigidbody2D)), so if they drifted apart, the wrap/companion tests would still pass while clouds stopped moving in game. A one-line identity assert in the spawn test would cover it.
| first menu button. | ||
| - pygamine 0.28.0: buildings, clouds and tiles use the engine's `require_component()` and | ||
| `require_image()` instead of `get_component()` plus a `None` assert, and the volume label uses | ||
| - pygamine 0.28.0: buildings and clouds keep their Rigidbody2D as a typed `rigidbody` attribute |
There was a problem hiding this comment.
Review: this bullet still credits pygamine 0.28.0, but after this PR the game no longer uses require_component() or require_image() from 0.28.0 (only clamp01()). It also leaves out the tile.py change. Suggest wording it as an internal change, separate from the 0.28.0 bump, or dropping it, since it's behavior-neutral.
| from pygamine import Application | ||
| from pygamine import clamp01 | ||
| from pygamine import AssetManager | ||
| from pygamine import clamp01 |
There was a problem hiding this comment.
Review (conventions): this hunk only moves an import. highrise/CLAUDE.md says: "Keep pure reformatting out of feature commits". It's tiny, but it's the only change in this file. Either drop it or put it in its own commit.
The review on this PR found the stored rigidbody attribute riskier than the property already on main (it can drift from the registered component). Keep main's src as is; tests/test_cloud.py now uses require_component(Rigidbody2D) instead of the Optional get_component(), which is what #23's review asked for. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
umutcanekinci
changed the title
|
Review follow-up (lead): narrowed the PR to the tests only. Findings on building.py, cloud_container.py, CHANGELOG.md and game.py no longer apply: those files are back to main, where |
Follow-up to #23. Its review asked that
tests/test_cloud.pystop treatingcloud.get_component(Rigidbody2D)(Optional) as non-None. The fix (3834a4e) was pushed in the same minute #23 merged, so it never reached main.That commit also swapped the
require_componentlookups insrc/for a storedrigidbodyattribute. The review on this PR showed that's riskier (the attribute can drift from the registered component), so the PR is narrowed:src/stays as on main, and the tests userequire_component(Rigidbody2D).Tests only, no gameplay change. 247 tests pass, ruff clean.
🤖 Generated with Claude Code