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

sdl: add autogenerated methods using genmethods tool #5

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

mewmew
Copy link
Contributor

@mewmew mewmew commented Feb 2, 2025

The genmethods tool is available at https://github.com/mewspring/genmethods

Also, update examples to use methods.

@mewmew
Copy link
Contributor Author

mewmew commented Feb 2, 2025

To generate the methods.go source file, run:

# install genmethods tool
go install github.com/mewspring/genmethods@master
# navigate to purego-sdl3 repository
cd /path/to/purego-sdl3
# generate methods.go source file
genmethods > sdl/methods.go

The genmethods tool can be updated, once we decide what methods names should be simplified (e.g. DestroyTexture -> Destroy).

From https://github.com/mewspring/genmethods/blob/2173b27db50e61e30d7ae5bbb33d9bf4419a308b/main.go#L202:

var renameMethod = map[string]string{
	// Renderer methods
	"DestroyRenderer":    "Destroy",
	"RenderClear":        "Clear",
	"RenderPresent":      "Present",
	"SetRenderDrawColor": "SetDrawColor",
	// Surface methods
	"BlitSurface":    "Blit",
	"DestroySurface": "Destroy",
	"LockSurface":    "Lock",
	"UnlockSurface":  "Unlock",
	// Texture methods
	"DestroyTexture": "Destroy",
	// Window methods
	"DestroyWindow":       "Destroy",
	"GetWindowSurface":    "GetSurface",
	"HideWindow":          "Hide",
	"ShowWindow":          "Show",
	"UpdateWindowSurface": "UpdateSurface",
}

@mewmew
Copy link
Contributor Author

mewmew commented Feb 2, 2025

Just for reference, and to keep it all in once place, adding comments from #3 (comment):

SDL3 already has a huge API and I think adding extra methods, would add too much complexity in maintaining the library.

That's fair!

I think we can still figure out a way to handle it without increasing maintenance burden.

As a proof of concept, I wrote a small tool that autogenerates the methods by parsing the Go source code of purego-sdl3.

Have a look at the approach taken in #5.

Closing this PR, as it's now superseded by #5.

@JupiterRider
Copy link
Owner

@mewmew Thank you very much! I think having a extra file for methods is a really clean idea.

I'll keep this PR open, so other people have the chance to tell their opinion too.

@clseibold
Copy link

clseibold commented Feb 2, 2025

This looks like a good way to do this while keeping maintenance requirements lower to me.

@mewmew
Copy link
Contributor Author

mewmew commented Feb 3, 2025

The genmethods tool has now been updated (mewspring/genmethods@024577d) to add comments for the generated methods.

See 844a4f7 for example output.

@mewmew
Copy link
Contributor Author

mewmew commented Feb 9, 2025

@mewmew Thank you very much! I think having a extra file for methods is a really clean idea.

I'll keep this PR open, so other people have the chance to tell their opinion too.

Happy we found a good approach that still keeps maintenance low : )

@JupiterRider, are we ready to merge this?

Hehe, at least @clseibold also agree that this looks like a good approach.

Once this PR is merged, I'll be able to switch over to purego-sdl3 for my personal use : )

Exciting times to come!

@mewmew
Copy link
Contributor Author

mewmew commented Feb 9, 2025

A go:generate directive is now added, so to re-generate the methods.go file, simply run go generate (ref: e629ef4).

Also, methods for the *Camera type are now generated (ref: mewspring/genmethods@29c64fc and 0fba11e).

@JupiterRider
Copy link
Owner

JupiterRider commented Feb 11, 2025

@mewmew What are the benefits of having methods? Usually they only make sense when you implement interfaces or using encapsulation.

purego-sdl3 is a binding and not an abstraction layer.

@clseibold
Copy link

clseibold commented Feb 12, 2025

@JupiterRider These aren't real "methods" in the traditional OOP sense. But anyways, one could say it's more idiomatic to Golang to use them. It also fits along nicely with every other golang library that uses "methods".

I don't think generating methods like this is much of an abstraction, and of course bindings can also be "abstraction layers" if one desires them to be. The only restriction is what you choose to place on it.

@mewmew
Copy link
Contributor Author

mewmew commented Feb 12, 2025

@mewmew What are the benefits of having methods? Usually they only make sense when you implement interfaces or using encapsulation.

I personally think it simplifies the API. When you have a sdl.CreateFoo function. Then, things you can do with Foo are categorized and kept separate from what you can do with Bar. Also, creation/destruction are handled in a similar fashion as one would do e.g.

f, err := os.Open("foo.txt")
if err { ... }
defer f.Close()
// instead of:
//    defer os.CloseFile(f)

But I realize that it comes down to personal preference.

So, if you prefer not to have methods, that's fine.

At least with genmethods it shouldn't take away time from main development of the purego-sdl3 project.

I can also be convinced otherwise, that having more than one way to interact with the library is distracting, and it's better to just provide a single way.

@emad-elsaid
Copy link

I personally also prefer this approach (it's a personal preference still). but some of the benefits that I expect:

  • The autocomplete menu in IDEs gets smaller as it contains only methods relevant to the variable/type
  • Chain call methods instead of enclose them as parameters like X.DoA().DoB().DoC() in cases where methods return the same instance. instead of Doc(DoB(DoA(X))) <- the call is in reverse logical order.
  • It'll be similar to go-SDL2 package. so easier for its users to migrate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants