Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Scene improvements #1651

Closed
einarf opened this issue Mar 20, 2023 · 11 comments
Closed

Scene improvements #1651

einarf opened this issue Mar 20, 2023 · 11 comments

Comments

@einarf
Copy link
Member

einarf commented Mar 20, 2023

The current sprite_lists and name_mapping members are "public". Some users are modifying these directly causing issues that are hard to debug and understand. We should make them private and add additional tools if needed.

arcade/arcade/scene.py

Lines 30 to 32 in 98d4cac

def __init__(self) -> None:
self.sprite_lists: List[SpriteList] = []
self.name_mapping: Dict[str, SpriteList] = {}

Things like __len__ and __delitem__ are candidates here.

On a few occations users have been deleting spritelists from name_mapping thinking that this will remove the spritelist. For more dynamic worlds this means you end up with hundres of spritelists in spritelists and performance tanks.

@einarf einarf added enhancement performance Speed / Optimizations labels Mar 20, 2023
@einarf einarf added this to the 3.0 milestone Mar 20, 2023
@einarf
Copy link
Member Author

einarf commented Mar 20, 2023

Another source of bugs is that add_sprite_list doesn't yell if you are adding a spritelist with an existing nane. It will overwrite the existing dict entry and append the spritelist to spritelists. This means games can easily lose track of spritelists and the game will grind to a halt over time.

We should also cover all these cases in unit tests.

@einarf einarf changed the title Scene: Make members private Scene improvments Mar 20, 2023
@einarf einarf changed the title Scene improvments Scene improvements Mar 20, 2023
@gran4
Copy link
Contributor

gran4 commented Apr 2, 2023

You can't make things in python private. I think the best way to go about this is to give warnings and comments in the code. I am also trying to getting to detect when it is happening.

@einarf
Copy link
Member Author

einarf commented Apr 2, 2023

You can't make things in python private. I think the best way to go about this is to give warnings and comments in the code. I am also trying to getting to detect when it is happening.

I know. I guess non-public is a better word. Just starting the name with an underscore. Most programmers know this means you shouldn't really access it or that there might be some bad consequences for doing so. Trying to avoid this happening is usually not worth it. (PEP 8)

We do this everywhere in the arcade codebase.

gran4 pushed a commit to gran4/arcade that referenced this issue Apr 2, 2023
_
improved scene, made it 2 variables non-public(issue pythonarcade#1651)
Cleptomania pushed a commit that referenced this issue Apr 3, 2023
improved scene, made it 2 variables non-public(issue #1651)

Co-authored-by: Grant Hur <22hurg@sjchrisitan.org>
@gran4
Copy link
Contributor

gran4 commented Apr 5, 2023

@einarf I think you can close this.

@pushfoo
Copy link
Member

pushfoo commented Apr 5, 2023

Things like __len__ and __delitem__ are candidates here.

I'd like to take another look at this before closing.

@Cleptomania
Copy link
Member

I think adding __len__ and __delitem__ would be good. They should be fairly easy to add so no reason not to.

@tiffanyxiao
Copy link
Contributor

@Cleptomania are you all at PyCon? Would love to try tackling this issue if someone hasn't already claimed it. I tried finding the Arcade group in 250D but was unsuccessful.

@Cleptomania
Copy link
Member

@tiffanyxiao Just missed us! We went out to lunch but will be back this afternoon. I’ll also be around for Tuesday and Wednesday

@pushfoo
Copy link
Member

pushfoo commented Apr 24, 2023

Would love to try tackling this issue if someone hasn't already claimed it.

Your help would be appreciated!

@gran4
Copy link
Contributor

gran4 commented Apr 29, 2023

What else needs to be added to this?

@Cleptomania
Copy link
Member

I think everything we wanted to hit is done with this. If there are any further desired improvements they should get their own smaller scoped issues now.

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

Successfully merging a pull request may close this issue.

5 participants