Allow erasing pixels in pygame.Surface.scroll and add repeat functionality

[damusss]

The shifting was incorrect, not setting the space created by the shifted pixels to black. Now it does that [EDIT: Now it does that only if the erase argument is True], but most importantly I think I implemented this feature idea: #2159 implementing a way to allow for repeated shifting. The video proof is here: https://discord.com/channels/772505616680878080/772940667231928360/1240055647584911443

I would love feedback about the C code as I did quite a lot of pointer operations.
Why?

• The erasing functionality is probably expected to happen so it's nice to have it
• The repeat functionality gives to this function some reason to exist
• It's cool

Question: how can I add a test for it? Edit: I modified the test, tell me if it's incomplete

You can use the following code with my request to test locally:

import pygame
pygame.init()
win = pygame.Window(size=(700, 700))
win.get_surface()
clock = pygame.Clock()

surf = pygame.image.load("B:/py/contributing/test.png").convert_alpha() # your image
surf = surf.subsurface(surf.get_bounding_rect())

main = pygame.Surface((600, 600))
main.blit(surf, surf.get_rect(center = (main.get_width()/2, main.get_height()/2)))
#main.set_clip(pygame.Rect(50, 50, 400, 400))

REPEAT = True
ERASE = True
SPEED = 4

while True:
    for e in pygame.event.get():
        if e.type == pygame.QUIT:
            pygame.quit()
            exit()
            
    win.get_surface().fill("black")
    screen = win.get_surface()    
    
    keys = pygame.key.get_pressed()
    dir = [0, 0]
    if keys[pygame.K_LEFT]:
        dir[0] -= SPEED
    if keys[pygame.K_RIGHT]:
        dir[0] += SPEED
    if keys[pygame.K_UP]:
        dir[1] -= SPEED
    if keys[pygame.K_DOWN]:
        dir[1] += SPEED
    main.scroll(dir[0], dir[1], ERASE, REPEAT)
    
    screen.blit(main, main.get_rect(center=(win.size[0]/2, win.size[1]/2)))
    
    win.flip()
    dt = clock.tick(60)/1000
    win.title = str(clock.get_fps())
    

[damusss]

Could the 2 faulty tests be rerun?

[MyreMylar]

The shifting was incorrect, not setting the space created by the shifted pixels to black.

Why do you think this is incorrect? The docs currently say:

Move the image by dx pixels right and dy pixels down. dx and dy may be negative for left and up scrolls respectively. Areas of the surface that are not overwritten retain their original pixel values.

Which looks to me like what the current behaviour is.

e.g.

Current main branch scroll a 200 pixel wide surface right by 100 pixels:

After this PR:

I think we should retain the current behaviour as the default, and if we want the original pixels not overwritten by scrolled pixels cleared to a colour (and why specifically black?) that should be an option/parameter. Otherwise we will probably accidentally break somebody's code.

---

As an addendum - do we think this function is likely to be a performance sensitive one like blit, or one used less frequently? I'm wondering whether it should support keyword arguments or not, now it has 3 params (possibly 4 after the above).

[MyreMylar]

See comment above.

[damusss]

@MyreMylar yeah that was a misunderstanding on my side. I don't think 4 arguments are too much. If they are, what do you suggest? Another function for repeat? only positional?

[bilhox]

Looking at how you implemented it, I believe the best option would be to use the same system used for flags on surfaces and window. Like that we can a fair amount of arguments for this function.

[damusss]

Looking at how you implemented it, I believe the best option would be to use the same system used for flags on surfaces and window. Like that we can a fair amount of arguments for this function.

That would require 2 new constants pygame.SCROLL_REPEAT and pygame.SCROLL_ERASE i believe. the new arguments would go from 2 to 1, i guess

[bilhox]

Looking at how you implemented it, I believe the best option would be to use the same system used for flags on surfaces and window. Like that we can a fair amount of arguments for this function.

That would require 2 new constants pygame.SCROLL_REPEAT and pygame.SCROLL_ERASE i believe. the new arguments would go from 2 to 1, i guess

I prefer this approach.

[bilhox]

These conditions are not necessary, you can calculate the result directly.

Suggested change

	if (dx >= w || dx <= -w) {
	dx = dx % w;
	}
	if (dy >= h || dy <= -h) {
	dy = dy % h;
	}
	dx = dx % w;
	dy = dy % h;

Furthermore, this has to be done when repeat is enabled which is not the case here.

[damusss]

It's not necessary. When dx or dy are >= w or h, and there is no repeat, that function returns early, so that code is never reached.

I see that I can remove the conditions tho, thanks.

I also have to test a specific case that just came to my mind.

[bilhox]

I think there were separate functions for the actual process of scrolling and the function definition, what about doing this to keep some kind of readability ?

[bilhox]

After a little digging with @damusss about some issues, here are the potential problem we are facing out currently:

• erase mode doesn't work when we have an alpha channel.
• There are missing pixels to erase sometimes, see for example this video (at 00:04, see at bottom right) :

surface_scroll_erase.mp4

[bilhox]

Code example so you understand how it simplifies the life a lot : https://gist.github.com/bilhox/ce024b994e536338a750a760024b5939