sdl/surface: I wonder if Surface.set should return an error and not panic

[jonathaningram]

Hi there! See https://github.com/veandco/go-sdl2/blob/ec14881f3097f4a0b38fc904ded7bea5e042a860/sdl/surface.go#L566

Do you think the signature of this method should be

func (surface *Surface) Set(x, y int, c color.Color) error {}

?

I was reading through the implementation while working out how to call this method in my program and it makes me nervous that this panics.

Could it instead return an error:

return fmt.Errorf("could not set pixel, unknown surface format %d", surface.Format.Format)

?

[veeableful]

Hi @jonathaningram, it needs to not return error in order to keep compatibility with draw.Image interface. We could add a second version called Set2() which returns an error. Do you think that is a good idea?

[jonathaningram]

Thanks @veeableful I had overlooked that. In that case I don't think adding a Set2(x, y, color) error is the solution.

Does this library need to be concerned with making *sdl.Surface satisfy the draw.Image interface?

Could callers just wrap *sdl.Surface and callSet` themselves (ignoring the error). So in a userland program:

type MySurface struct {
    s *sdl.Surface
}

// compile time check to make sure it satisfies the interface
var _ draw.Image = (*MySurface)(nil)

func (s *MySurface) Set(x, y int, color color.Color) {
    // where the current `Set` method would now return an error
    if err := s.s.Set(x, y, color); err != nil {
        panic(err)
    }
}

// other methods to satisfy the interface, e.g. At

[veeableful]

No problem @jonathaningram. sdl.Surface implements the draw.Image interface so other libraries that work with draw.Image interface can work with it as well. There does seem to be some work left before it can handle all the essential pixel formats. Is there any particular problem that is blocking you at the moment? I could try to fix that first when I have some time later!

We also try to leave things be at the moment (or add new API if necessary) as I'm aware that there are people and tutorials that depend on the current API so breaking them would inconvenience many people.

[jonathaningram]

@veeableful no issue specifically—just the panic that I was concerned about originally, but nothing is stopping me.

I understand the concerns around backwards compatibility and I wouldn't want to be the one to be breaking it either—though having said that, while this library is in the <v1.0 stage would be the best time to do these breaks and I (personally) think that it's acceptable for go-sdl2 to break things during this time in order to release the best API for a v1.

I'm going to close this issue. If you want to reopen it to track the panicking code, feel free to do so. Otherwise there's no action at this time.

P.S. thanks for an awesome library.

[veeableful]

@jonathaningram I see.. in that case, I'll add some of the missing cases in the switch statements and tests later to make it harder to trigger the panic case. Hopefully it will eventually remove the need for the panicking default case in the switch statement and therefore remove the need to break API :stuck_out_tongue:

And thank you for the compliment and using this library! I'm glad you are enjoying it :relaxed: