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

ebiten: remove *ebiten.Image functions that are needed for image.Image #2945

Open
11 tasks
hajimehoshi opened this issue Mar 28, 2024 · 12 comments
Open
11 tasks

Comments

@hajimehoshi
Copy link
Owner

hajimehoshi commented Mar 28, 2024

Operating System

  • Windows
  • macOS
  • Linux
  • FreeBSD
  • OpenBSD
  • Android
  • iOS
  • Nintendo Switch
  • PlayStation 5
  • Xbox
  • Web Browsers

What feature would you like to be added?

*ebiten.Image now implements image.Image which includes At and ColorModel. Especially At is problematic as At seems handy though it is very expensive to sync pixel data from GPU to CPU. So, we don't want encourage users to use At. *ebiten.Image implementing image.Image was not a good API in terms of execution costs.

Thus, we propose these API changes.

  • Remove (*ebiten.Image).At
  • Remove (*ebiten.Image).RGBA64At
  • Remove (*ebiten.Image).ColorModel
  • Remove (*ebiten.Image).Set
    • This was for draw.Image, but as *ebiten.Image is no longer image.Image, I don't think we have to have this.
  • Let (*ebiten.Image).SubImage return an *ebiten.Image instead of image.Image
    • This followed the convention of the standard libraries, but as *ebiten.Image is no longer image.Image, we no longer need to follow this.
  • Let (*ebiten.Image).ReadPixels return an error
    • This was because At couldn't return an error and Ebitengine accumulated an error in the internal. As At is removed, we no longer need to have such a hack.
  • Add func Image(*ebiten.Image) image.Image to ebitenutil (or Screenshot or Dump?)
    • As we keep (*ebiten.Image).ReadPixels, we can put a utility function to create an image.Image in a utility package, rather than the core package.

As these are breaking changes, we will do in v3.

/CC @Zyko0 @superloach @tinne26

Why is this needed?

An expensive API should not be handy since people often misuse such APIs.

@hajimehoshi hajimehoshi added this to the v3.0.0 milestone Mar 28, 2024
@hajimehoshi
Copy link
Owner Author

#2902 might be related

@bjorndm
Copy link

bjorndm commented Apr 22, 2024

For an indexed bitmap editor for pixel art it is handy that ebiten.Image is an image.Image. If this is removed we will need to add conversion functions in both ways.

@hajimehoshi
Copy link
Owner Author

Yeah, this additional function call is an intentional change to make the execution cost more explicit.

@bjorndm
Copy link

bjorndm commented Apr 22, 2024

As long as indexed bitmaps are properly supported both ways, this is fine by me

@hajimehoshi
Copy link
Owner Author

I have no idea about the bitmap editor, so I cannot say anything about this.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented May 1, 2024

As ReadPixels is very special as it flushes the internal command queue, I might want to move this method to somewhere where we can call it in a limited situation (under Update or Draw). Actually, reading pixels was a blocker to implement handling events (#1704 (comment)).

Another possible idea is to make ReadPixels asynchronous:

type ReadPixelsResult struct {
    Bytes []byte
    Err   error
}

func (*Image) ReadPixels(buf []byte) (<-chan ReadPixelsResult)

@eihigh
Copy link
Contributor

eihigh commented May 2, 2024

Another possible idea is to make ReadPixels asynchronous:

I have a few questions:

  • What is the role of the buf argument in this case?
  • What is the case where ReadPixels returns an error, for example?
  • If we just add a return value, compatibility is certainly preserved, but if the signature is going to change, it would be more straightforward to change the function name.
  • I think the "non-blocking" functions in Go take chan as an argument, not a return value (sorry if I'm wrong). I was wrong, functions like time.After return chan.

@hajimehoshi
Copy link
Owner Author

What is the role of the buf argument in this case?

To avoid or reduce internal allocations.

What is the case where ReadPixels returns an error, for example?

The graphics driver might fail for some reasons. In most cases there is nothing a user can do.

If we just add a return value, compatibility is certainly preserved, but if the signature is going to change, it would be more straightforward to change the function name.

No, adding a returning value breaks the compatibility. There can be an interface that defines ReadPixels.

@eihigh
Copy link
Contributor

eihigh commented May 2, 2024

To avoid or reduce internal allocations.

Will the allocated buf be written from another goroutine?

@hajimehoshi
Copy link
Owner Author

Will the allocated buf be written from another goroutine?

Yes and no. We can treat this like append: if the buf is not enough, ReadPixels can extend the buffer.

@eihigh
Copy link
Contributor

eihigh commented May 2, 2024

OK, thanks for responding. To be honest I am not sure about the design of the asynchronous API, but my opinion is in favor of this proposal.

@eihigh
Copy link
Contributor

eihigh commented May 2, 2024

Another example of Go's non-blocking API is a Start/Wait pair like exec.Command, I'm not sure if it's appropriate for Ebitengine, but just FYI.

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

No branches or pull requests

3 participants