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

damusss commented 4 months ago

The shifting was incorrect, not setting the space created by the shifted pixels to black. Now it does that [EDIT: now it does that with the erase flag only], but most importantly I think I implemented this feature idea: https://github.com/pygame-community/pygame-ce/issues/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))

FLAG = pygame.SCROLL_REPEAT
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], FLAG)

    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 commented 4 months ago

Could the 2 faulty tests be rerun?

MyreMylar commented 4 months ago

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).

damusss commented 4 months ago

@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?