arcade.SpriteList "atlas" property sometimes does not exist

[bunny-therapist]

Bug Report

System Info

Run this and paste the output here: python -m arcade

Arcade 3.0.0.dev24

vendor: Intel
renderer: Intel(R) UHD Graphics
version: (3, 3)
python: 3.11.0 (main, Oct 24 2022, 18:26:48) [MSC v.1933 64 bit (AMD64)]
platform: win32
pyglet version: 2.0.8
PIL version: 9.4.0

Actual behavior:

The "atlas" property of SpriteList crashes with an AttributeError if there is no window or if the SpriteList is lazy and has not been fully initialized (init_deferred) yet. The reason is that "self._atlas" is not set at all in these cases.

The error is very deceptive: AttributeError: 'SpriteList' object has no attribute '_atlas'. Did you mean: 'atlas'?

Expected behavior:

I would expect calling the "atlas" property to, e.g., do one of

1. actually run init_deferred so there is an atlas to return
2. raise a better error (AttributeError for a private attribute makes it look like a programming error)
3. return None (this probably has too big consequences, so I would be wary of this).

Steps to reproduce/example code:

import arcade

class IssueDemonstrationWindow(arcade.Window):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        sl = arcade.SpriteList(lazy=True)
        sl.atlas


window = IssueDemonstrationWindow()
arcade.run()

[pushfoo]

I can replicate this. Thank you for writing the example.

To my understanding, automatic initialization would undermine the intent of lazy=True: avoid creating an atlas or any other GL-dependent abstractions. It's useful for CPU-only tasks.

Having the atlas property return None is acceptable in my opinion:

1. lazy=True is an advanced feature
2. We will already have an exception from trying to use a non-existent atlas, this just moves it somewhere else

I strongly agree the behavior should be explained better, including linking the relevant programming guide section from the SpriteList doc. That fell behind due to a mega-PR (#1620) which I need to rewrite or carve parts out of. My apologies.

At the moment, I think the best approach is ensuring self._atlas is always declared by removing the if statement here:

arcade/arcade/sprite_list/sprite_list.py

Lines 108 to 109 in 1e60dff

	if atlas:
	self._atlas: TextureAtlas = atlas

This has two benefits over the AttributeError approach:

1. Accurate typing: since atlas is exposed through a property which always exists, it seems misleading to raise an AttributeError here.
2. Better performance than adding more logic to the atlas getter

The unresolved question is how preload_textures should handle initialization. Do you have any thoughts on that at the moment? The doc doesn't cover it.

[bunny-therapist]

Is preload_textures even needed as a method of SpriteList when the atlas is exposed through a property? One can just add textures to the atlas directly (that is what I usually do) - if you are doing things such as messing with atlases and preloading textures, you are probably already an advanced user. Personally, I would just get rid of the preload_textures method, but I don't know the history of it or if other people are using it.

Instead of returning None (I see you had to add type: ignore comments in a lot of places - I expect I would have to do the same, or add if-checks, which I am not looking forward to), I was thinking of just raising a better error than AttributeError, like SpriteListNotInitializedError or something like that. But you say that adding logic to the atlas getter might have a performance cost, so I will trust you on that. (To me, that seemed insignificant, but you probably know better.)

[pushfoo]

tl;dr:

1. The exception approach is probably better despite being less explicit
2. The PR's contents were merged since the doc and unit test changes are useful
3. We should make follow-up PRs

Is preload_textures even needed as a method of SpriteList when the atlas is exposed through a property?

I'm not sure. Although atlases currently initialize GPU resources, they could be given deferred initialization support. That idea may have come up before in the context of atlas pre-packing tools.

Regardless, this is a separate issue and best discussed with @einarf since he has the most thorough understanding of the subject.

I expect I would have to do the same, or add if-checks, which I am not looking forward to

It was the most explicit option, although I agree it is very ugly.

was thinking of just raising a better error than AttributeError, like SpriteListNotInitializedError or something like that.

After discussing it with einarf on Discord, I agree with the exception approach. The None option is equivalent to it if you leave out the conditional checks.

Is there pre-existing exception type the new exception type could inherit from?

To me, that seemed insignificant

We need to translate the benchmarking examples cspotcode has posted into documentation.

but you probably know better.

I don't. That's why I tagged you on the PR to review it: I wanted you and einarf to tell me how / why it's wrong or risky.

[bunny-therapist]

After discussing it with einarf on Discord, I agree with the exception approach. The None option is equivalent to it if you leave out the conditional checks.

Only if I add type: ignore checks like in the PR.

Is there pre-existing exception type the new exception type could inherit from?

I looked in the SpriteList code and I only see Exception and (most frequently) ValueError being raised. I am not sure why ValueError was chosen for this, but it seems to be what is raised in situations like this.

Actually, if you look at the geometry property, it works exactly like I suggested atlas could work. It returns self._geometry if it exists, otherwise it raises ValueError complaining that the SpriteList is not initialized. (I am on 3.0.0.dev24.) Isn't this exactly the case we are discussing for atlas?

I don't. That's why I tagged you on the PR to review it: I wanted you and einarf to tell me how / why it's wrong or risky.

I don't know about "wrong", I was only commenting on your claim that there would be a performance hit to adding an if-check in the atlas property, if I understood you correctly. I feel that an if-statement would not cost much performance and that the value of being able to have TextureAtlas as return type to atlas rather than Optional[TextureAtlas] would be worth it. I am not sure what SpriteList.geometry is used for, but given that it already works this way, I suppose it could be used as a model to understand any possible performance issues.

[einarf]

Fix already merged