Skip to content

Improve Textures guide (#2821) - #2832

Open
MinhBDoan wants to merge 1 commit into
pythonarcade:developmentfrom
MinhBDoan:fix-texture-docs
Open

Improve Textures guide (#2821)#2832
MinhBDoan wants to merge 1 commit into
pythonarcade:developmentfrom
MinhBDoan:fix-texture-docs

Conversation

@MinhBDoan

Copy link
Copy Markdown

This PR addresses all requested improvements from issue #2821:

  • Reworded "sprite engine" to describe hit box usage
  • Added Texture Layers section (Arcade / pyglet / OpenGL)
  • Clarified Texture Uniqueness:
    • Defined what "unique" means
    • Explained consequences of duplicate names
    • Explained flipping options
    • Added cross-reference to arcade.load_texture
    • Referenced arcade/cache/texture.py
  • Updated Texture Cache section:
    • Clarified name/hash attribute
    • Cross-referenced TextureCache class and flush() method
    • Included example code

@MinhBDoan

Copy link
Copy Markdown
Author

“This PR addresses all requested changes from #2821. First-time contribution, feedback welcome!”

@pvcraven

pvcraven commented Aug 6, 2026

Copy link
Copy Markdown
Member

Claude review:

Review: Improve Textures guide (#2832)

Thanks for tackling #2821 — the restructuring (Texture Layers section, expanded Uniqueness/Cache sections) is a good shape and hits the checklist. However, I traced the new technical claims against the current source and found a few factual inaccuracies that should be fixed before merge, since the whole point of this PR is correctness/cross-refs.

🔴 Issues to fix

  1. Wrong class referenced for default_texture_cache.

    Arcade stores texture instances in the default texture cache
    (:py:class:`arcade.cache.TextureCache`). To clear all cached textures,
    use the :py:meth:`arcade.cache.TextureCache.flush` method:

    default_texture_cache = TextureCacheManager() (arcade/texture/__init__.py:19) — it's a TextureCacheManager, not a TextureCache. The manager wraps a TextureCache internally, but default_texture_cache.flush() actually calls TextureCacheManager.flush() (arcade/texture/manager.py:64), which by default also flushes sprite sheets and image data (not just texture entries, and not hit boxes). arcade.TextureCacheManager is already a documented Sphinx target (doc/api_docs/api/texture.rst:25) — use that instead:

    Arcade stores texture instances in the default texture cache
    (:py:class:`arcade.TextureCacheManager`). To clear all cached textures,
    use the :py:meth:`arcade.TextureCacheManager.flush` method:
  2. flipped_vertically=True doesn't exist. Grepped the whole codebase — no such parameter/kwarg anywhere. The real API is the zero-arg instance method texture.flip_vertically() (arcade/texture/texture.py:482), called after loading, not a load-time flag.

  3. "the underlying pixel data changes" is incorrect. Texture.transform() (arcade/texture/texture.py:564-596), which backs all flip methods, passes the same self.image_data into the new Texture — only hit box points and a GPU-side draw transform differ. This directly contradicts flip_vertically()'s own docstring ("returns a new texture with the same image data"). Suggest rewording to something like: flipped textures get a different hit box (computed from the transformed pixel data) even though the underlying image data is shared.

  4. arcade.load_texture is probably the wrong cross-ref for cache-naming. arcade.load_texture (arcade/texture/loading.py:12) is explicitly docstring'd as "Load a texture from disk (no caching)." The unique-name/cache-key logic (Texture.create_image_cache_name, arcade/texture/texture.py:282) is used by TextureCacheManager.load_or_get_texture / _load_or_get_texture, not by plain load_texture. Worth pointing at default_texture_cache.load_or_get_texture instead. (Also: create_image_cache_name only combines path + crop, not flip state — another sign this is the wrong function to cite for "name includes flipping.")

🟡 Nit

  • Trailing double-spaces (Markdown-style hard breaks — meaningless in RST) on the **Arcade (Python / RAM)** and **pyglet** bullet lines. The repo's trailing-whitespace pre-commit hook (.pre-commit-config.yaml:10) will likely flag these.

Overall

Structure and intent are good; recommend re-verifying each new technical claim against TextureCacheManager / TextureCache / Texture.transform source directly rather than paraphrasing the issue description, then this should be good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants