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

Add sdl2_video types #2664

Merged
merged 4 commits into from Oct 19, 2021
Merged

Add sdl2_video types #2664

merged 4 commits into from Oct 19, 2021

Conversation

Starbuck5
Copy link
Contributor

@Starbuck5 Starbuck5 commented Aug 7, 2021

I wanted to keep making some progress on _sdl2, so I decided this was a good small thing I could get done.

With this, we have skeleton documentation and full typing for the _sdl2.video system.

By putting this in I don't mean to solidify the current system against future change, in fact I have several things I would like to change with it. But that can't happen until #2564 is resolved.

These were tested with mypy against mcpalmer's dancefont, tilemap, and Flyboy programs, along with the pygame/examples video.py program.

Also, I didn't know you could run black on pyi files, but you can. I used that to make it nice and pretty.

@Starbuck5
Copy link
Contributor Author

Questions from @MightyJosip posted on discord:

Great job 🎉 , this is one of the last things _sdl2 is missing before I can start trying it out. Didn't check if the all types are correct (tho I believe they indeed are, after all you are the one that wrote that code), but here are some of my general suggestions/questions after I briefly "reviewed" it:

  • line 10 Color and line 14 Rect can go without " ("" is used only if you write an object that is defined somewhere later in the code, and you have the declaration for those objects on line 5/7), tho it is probably left after you copy pasted that variables from other files.
  • Are those constants from line 23-28 called like pygame._sdl2.video.WINDOWPOS_UNDEFINED, or just pygame.MESSAGEBOX_ERROR?
  • line 56, I am not sure Iterable[int, int] actually means something with 2 elements (like Tuple[int, int]). Because Tuple uses _VariadicGenericAlias (only Tuple and Callable uses it in typing module), and Iterable uses _GenericAlias (same thing List uses, so imo Iterable[int, int] is same as Iterable[int]. If you want to limit stuff like warning on (x, y, z) than you would have to use the union of different types.
  • That same line appears on different places, so check it (also in later implementations it should probably accept pygame.math.Vector2 as well, something like this:
    _Coordinate = Union[Tuple[float, float], List[float], Vector2]
  • Compare kwargs on line 100 and 59 (having/not having **)
  • This is really minor thing, but is it intended for the users to manually change all class variables (for example line 99 can the color attribute become string for example). If this was pure python than yes, but I don't know how you implemented it in C where you can limit some stuff
  • Also how do you import the code inside of pygame._sdl2.video (is it auto imported if you write import pygame._sdl2 (asking because of this https://github.com/pygame/pygame/pull/2664/files#diff-6aff2b75181b00c78ed6d1fa4b945d1c5041648ffa7225de459393c68cc1b716)

@Starbuck5
Copy link
Contributor Author

Are those constants from line 23-28 called like pygame._sdl2.video.WINDOWPOS_UNDEFINED, or just pygame.MESSAGEBOX_ERROR?

The constants live in the pygame._sdl2 and pygame._sdl2.video namespaces.

line 56, I am not sure Iterable[int, int] actually means something with 2 elements (like Tuple[int, int]). Because Tuple uses _VariadicGenericAlias (only Tuple and Callable uses it in typing module), and Iterable uses _GenericAlias (same thing List uses, so imo Iterable[int, int] is same as Iterable[int]. If you want to limit stuff like warning on (x, y, z) than you would have to use the union of different types.

Ugh, I used that a lot. I'll have to look into it, but you're probably right.

Compare kwargs on line 100 and 59 (having/not having **)

Good spot. I think the no ** is correct, but I'll check around.

This is really minor thing, but is it intended for the users to manually change all class variables (for example line 99 can the color attribute become string for example). If this was pure python than yes, but I don't know how you implemented it in C where you can limit some stuff

This is one of the most weird areas of the current video API. Both implementations implement these as properties, if I remember correctly. I think the Cython one is only color objects, but mine can take any valid color data. The way I see it working is that you can set it using any valid color format, but when you get it out, it's always going to be a Color object.

Also how do you import the code inside of pygame._sdl2.video (is it auto imported if you write import pygame._sdl2 (asking because of this https://github.com/pygame/pygame/pull/2664/files#diff-6aff2b75181b00c78ed6d1fa4b945d1c5041648ffa7225de459393c68cc1b716)

Good question. There's an _sdl2 init.py: https://github.com/pygame/pygame/blob/main/src_py/_sdl2/__init__.py. That's how the _sdl2 stuff is actually exposed.

@MightyJosip
Copy link
Contributor

One extra thing

  • I guess this line should be dstrect: Optional[_CanBeRect, Iterable[int]] = None

About the color thing right now type hints accepts Image.color = "a", but when you run the code you get ValueError: attempting to assign sequence of length 1 to slice of length 3, or TypeError: color components must be integers if you try to assign 'aaa'

Constants are ok the way they are written right now, also the import thing as well

@Starbuck5
Copy link
Contributor Author

Starbuck5 commented Aug 8, 2021

Ok, I fixed the "easy stuff," which is everything except the Iterable[int, int] situation. I don't know how to deal with that. It's too finicky. It's hard to see how the Cython code will fail, because it's so slippery with type conversions between C and Python. And edge cases that work on that probably won't work on my C implementation. Like if you pass a position as [100, 200, "gobblygook", "pygame", 37], that will probably work in Cython and fail in C.

About the color thing right now type hints accepts Image.color = "a", but when you run the code you get ValueError: attempting to assign sequence of length 1 to slice of length 3, or TypeError: color components must be integers if you try to assign 'aaa'

Cython implementation needs Color objects, C implementation is fine with anything iirc. I guess the types should be accurate, not aspirational, so I fixed it.

@Starbuck5
Copy link
Contributor Author

Questions from @zoldalma999 posted on discord:

Hey Starbuck, I have a few questions/comments about your pr, 2664 (_sdl2.video types):

  • fullscreen_desktop did not get types (line 58)
  • kwargs can be bool I think (line 59)
  • self is missing in set_modal_for (line 82)?
  • afaik Iterable[int, int] is not valid. It should be Iterable[int] (line 55, 56, 77, 78, 88, 104, 106, 148, 158, 159)
  • I think staticmethods should get the decorator (I get this with pylance) (line 93, 141)

I might be wrong on some of these tho, but these are the things I noticed
Capture

@illume illume added SDL2 types Type hint checking related tasks labels Oct 16, 2021
@illume illume added this to the 2.0.3 milestone Oct 16, 2021
Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good. I think we can merge this one in?

(The typing mistakes can be done either in another PR or this one IMHO)

@Starbuck5
Copy link
Contributor Author

Ok, I think it's ready now.

I also realized I forgot the Window.from_display_module() method entirely.

I'm really the wrong person to be doing this PR since I don't use typing at all, and I don't even understand how to check if it's right half the time, but I wanted to keep making progress on _sdl2 somehow.

@Starbuck5 Starbuck5 merged commit 649ba5b into pygame:main Oct 19, 2021
@Starbuck5 Starbuck5 deleted the sdl2-video-types branch October 19, 2021 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
SDL2 types Type hint checking related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants