A start on linting the sprite module #1925
Conversation
|
If anyone has any ideas for the last two warnings I'd like them. |
|
Reasons can be added after class Sprite(object): # pylint: disable=useless-object-inheritance; required for python 2 |
…les about to be added to setup.cfg
| return _ret | ||
|
|
||
| @staticmethod |
There was a problem hiding this comment.
Nice improvement moving these to separate functions.
I haven't read through these two to see if they were refactored correctly however.
To Do:
|
…d-only, layer property to DirtySprite to avoid protected access
|
Added The property uses I also edited the tests slightly because they seemed to be directly assigning values fruitlessly to It's probable that there is other code out there right now directly assigning values to Where the groups set the layer of a sprite they now use Let me know if I'm wrong or if this is too controversial. |
|
Awesome. I haven't had a chance to read it yet, but I will soon. |
|
Thinking about a couple of things... So that the
Should layer be read only? |
|
Here's some scripts to test with. python3 -m pygame.examples.testsprite
python3 -m pygame.examples.testsprite -layered_dirty
python3 -m pygame.examples.testsprite -layered_dirty 5000
python3 -m pygame.examples.testsprite -static
python3 -m pygame.examples.testsprite -noupdate_rects
python3 -m pygame.examples.aliens
python3 -m pygame.examples.chimp
python3 -m pygame.examples.maskThere's also this https://gist.github.com/illume/41a065bf92201330666b7d8a20a634ca that plots fps over 2000 frames. |
|
Ran a few tests with python 3.8... TLDR; benchmarking is hard.
Full script: # Run it five times. Sleep inbetween so CPUs/GPUs can cool down.
timesarray=( one two three four five)
checkouts=( master 0e42b3624ba24eacf8d9b6741e330ead0a234dd3 f79e1ca226927f60730ef5f5203a06da0d1442ac 5bc350d0446fcb5b843d0c8e0af2537c414d4632 f5c3fe3afbcfaa0362ba1e304acff3d18a853406 d27774b7a57b74d05d80e82b630b6f443351c434 a9d72d9543fc83075aa5a0ab04260b6873677e3a )
for checkoutit in "${checkouts[@]}"
do
git checkout $checkoutit
python3 setup.py install 2> /dev/null > /dev/null
# So pygame loaded in file cache memory, and first run is not affected.
timeout 1s python3 -m pygame.examples.testsprite 2> /dev/null > /dev/null
for ti in "${timesarray[@]}"
do
timeout 20s python3 -m pygame.examples.testsprite -layered_dirty 5000 2> /dev/null | grep FPS
sleep 30
done
for ti in "${timesarray[@]}"
do
timeout 20s python3 -m pygame.examples.testsprite 2> /dev/null | grep FPS
sleep 30
done
sleep 30
done
|
No, I think that would probably be more confusing because the leading underscore generally indicates a protected variable only to be used inside the class or for inherited classes. Generally users won't be using '.layer' or '._layer' in performance critical code anyway because it is designed to just be a read only indication of the sprite's layer, changing the layer variable doesn't change a sprite's actual layer in a group so there is no point setting the layer variable in a tight update loop or anything like that. Actual layer management is done through the groups which have layers and the layer property of a sprite is a convenience thing for reading the current layer of a sprite. The whole concept of a sprite having a layer variable is a little bit odd as sprites can be in multiple groups. If a sprite is present in multiple groups with layers, on a different layer in each group - what value should the sprite's layer (.layer or ._layer) variable have? Usually it doesn't come up because people don't use multiple groups that way, but it is still a little odd.
Yes, given the current way the code works - because otherwise it creates confusion when you set the layer of a sprite and it doesn't change layers in (one of) its groups. The restriction makes sense in the docs and given how layers actually work in the code, the change over to making it a property just enforces things. Making sure that people incorrectly assigning a value to a sprite's layer will now get a warning that what they are doing won't actually do anything. I'm still sure it'll cause someone's code somewhere to raise an error because a lot of people make a mess when they are coding. A more dramatic change would be to change the I think the way I've done it makes the most sense that can be made of it under the current code logic but these are definitely questions to ponder when (if?) there is a sprite redesign after 2.0. |
Looks like there isn't any dramatic change in performance with this benchmark which I guess is a win? |
…import of Rect & get_ticks to try and speed up LayeredDirty draw a smidge
|
Testing all these:
After lots of test runs they all seem basically the same except the LayeredDirty ones which seem a teeny bit slower in the new version versus the old mega function. This one: Seems to run at about 21 FPS on average with the current mega draw() function, but at about 20 FPS on average with the new split into two methods arrangement. It's tricky to be 100% with a small change like this but it seems fairly consistent so far. I'm not sure exactly what is causing the change so far, other than that it is in the LayeredDirty draw() method. I tried a few small things but didn't have much luck (the latest commit reintroduces a too-many-locals pylint error trying to chase that missing frame). It may be I have to abandon refactoring this one and just revert to the old slightly faster mess for now to get this checked in. I'll poke at it again tomorrow for a bit with a fresh brain and some biscuits. Everything else seemed fine though. |
|
Probably it's worth committing your cleaner version, over the messy one IMHO. If we can't detect a difference at the macro level, I think it's fine :) On the other hand, if you want to continue... It could be worth doing a benchmark at the method level with just the one you found a bit slower. Perhaps even do some profiling to see if you notice anythin. |
…dded to groups. fix hasattr checks.
| if not self.alive(): | ||
| setattr(self, '_layer', value) | ||
| else: | ||
| raise AttributeError("Can't set layer directly after " |
There was a problem hiding this comment.
Did you consider changing the layer in each group here?
Something like...
[group.change_layer(self, value) for group in self.groups]There was a problem hiding this comment.
Yeah, I discussed it a bit further above. One implementation difficulty is that not all types of groups have layers so the change_layer() method doesn't exist on those groups and will throw an error with an implementation like the above. A fairly common use case might be where people have some subset of sprites in a layered group, but also have all their sprites in one single basic mega-group for convenience. Probably we could work around that with a more complicated setter.
It's also a slightly more dramatic functionality change which I'm mostly steering clear of in these linting passes, and I guess we'd also need to update the docs to reflect that you can now change the layer of a sprite by setting it's layer. I'm sort of ambivalent on it for now, unless you think it's super useful?
I poked at this a little and noticed that right now with both the current layered_dirty tests the draw method goes down the slower rendering path which seemed wrong based on the intentions. A little investigation later showed that this was partly because That turned out to be because the testsprite example was not calling Anyway since that was obviously bad, the easiest quick fix without tearing down the whole house is just to raise the default threshold a chunk for when it switches to the other (currently slower) path. With that change this test: Now runs twice as fast after the refactor. I expect we should add to one of those issues discussing a new sprite class whether the Dirty flag technique is still useful at all, and if it is we need a test that demonstrates its worth because this one with current blitting speeds seems to show that so far the time spent setting it up costs more than the time saved in all circumstances, at least in SDL2. I'll have a check with SDL1 as well to see if it makes a difference. |
|
Looks like it is always faster to use the non-dirty rect version of draw always on SDL1 as well. Interesting, think I need to go and make some changes to my gui module. |
…hing to using dirty rect technique to 999 as it seems to be always slower.
MyreMylar
added
the
needs-testing
|
I've noticed bugs in the thresholding code as well. The speed of dirty/not-dirty is really dependent on the hardware and the images drawn. The goal of that threshold code is to select the appropriate drawing mode for that scene on that hardware. |
|
it'd be useful to get some more testing on different platforms with the default threshold change using: those sets of settings (though I personally set it up via Pycharm's Run->Edit configuration to make it work). Basically comparing 2.0.0.dev10 or master versus the latest on this branch just in case it only runs faster on windows. To run the test you just have to let it run for a few seconds (I was counting to ten) then close the window and it should report the average FPS. For me the difference was very noticeable with current master, sdl2 config showing about 20ish FPS on the 5000 test and showing more like 40ish FPS with the latest on this branch just from the threshold change. If you can't be bothered to sync to this branch you should get the same results by changing line 214 of |
|
Did you see there is a static option? LayeredDirty works best when there are static(non-moving) sprites. I'm thinking it might be best to factor out the threshold algorithm into a function, so it can be tested easily? This would be easier than testing different apps on different platforms. |
|
Note, that on windows with SDL < 2.0.13 partial updates are not implemented by SDL. |
Tried running the testing again with the static flag as well. It went a bit better for the default threshold (i.e. using DirtyFlag) but was still slightly worse than not using it. But eh, I think it is probably not the time to puzzle this out on windows with SDL 2.0.12. I'll just leave it at what it was before. |
MyreMylar
removed
the
needs-testing
|
My conclusion is that I'll stop messing with LayeredDirty's draw function. I don't think I've made it worse in any significant way performance wise (though it's hard to tell on windows). I think changing the way layers are set is potentially a decent change to sprite's functionality if it can be made to work, and having properties will certainly make it easier to do that - but probably should be in a separate focused PR that changes the docs and adds tests rather than this linting focused one. Is there anything else important to address? |
|
Some future work came our of this PR, it would be good to capture it and make issues.
|
illume
added
Code quality/robustness
Warnings disabled = 5:
layer=None # noqa pylint: disable=unused-argument; supporting legacy derived classes that override in non-pythonic wayreturn self.__class__(self.sprites()) # noqa pylint: disable=too-many-function-args; needed because copy() won't work on AbstractGroupclass collide_circle_ratio(object): # noqa pylint: disable=invalid-name; this is a function-like classclass collide_rect_ratio: # noqa pylint: disable=invalid-name; this is a function-like classdef draw(self, surface, bgd=None): # noqa pylint: disable=arguments-differ; unable to change public interfaceRemaining issues after this PR:
Apart from the TODO which I'm ignoring, the remaining issues are:
__init__.pyor something. This isn't pygame's general coding style for modules though so I'm not sure whether we should just leave it as it is and disable the warning or divide up the module in some other way...