[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 with the erase flag only], 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))

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]

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]

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

[MyreMylar]

All looks good to me now, the three modes work as I expect and add some nice functionality to this function which I expect is not often used in it's current form.

Perhaps it will see a resurgence? 👍

[bilhox]

except what I mentionned, nice, it works correctly now !

[bilhox]

you removed the old text, so what is the "normal behaviour" ?

[damusss]

You really don't need to request changes when some requested changes are already in action you know that right 😭

does this satisfy you?

[bilhox]

LGTM !

[damusss]

btw I really suggest squash and merge, 32 commits are not worth this change!!

[bilhox]

except myremylar, we're only 2 who contributed to this PR, so I'm waiting tomorrow before merging it.