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: add a new image API to 'deallocate' its internal state and deprecate Dispose #2808

Closed
11 tasks
hajimehoshi opened this issue Oct 11, 2023 · 7 comments
Closed
11 tasks
Labels
Milestone

Comments

@hajimehoshi
Copy link
Owner

hajimehoshi commented Oct 11, 2023

Operating System

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

What feature would you like to be added?

If an image is disposed, the image is no longer available. While explicit dispoing is necessary for actual games, I don't like the exisistence of this additional state in an image. Actually, this prevents us from introducing some optimization (#825 (comment)). (EDIT: Now Dispose does nothing for a subimage, we might be able to introduce this optimization.)

There are some possible solutions:

  1. Changing the meaning of (*Image).Dispose. The new Dispose disposes the internal state of an image, but an image is still available as a cleared image. internal/atlas state is recreated when the image is used again. This is a breaking change, so this would be introduced in v3.
  2. Changing the behavior of (*Image).Clear. If the image is not used for a while after Clear, Ebitengine can dispose its internal state. If the image is used again, internal/atlas state is recreated like 1. This is not a breaking change so we can introduce in v2, but we have to be careful the performance impact. Also, the new Clear might be confusing to users. Dispose is deprecated in v2 and removed in v3.
  3. Introducing a new function like (*Image).Clear2 or a better name, and deprecating (*Image).Dispose. Clear2 is the same as the new Deprecate in 1.

Why is this needed?

In order to simplify the model of an image.

@hajimehoshi
Copy link
Owner Author

For 3., what about (*Image).Unmap for example?

@hajimehoshi hajimehoshi changed the title ebiten: removing the 'disposing' state of an image ebiten: removing the 'being disposed' state of an image Oct 11, 2023
@Zyko0
Copy link
Contributor

Zyko0 commented Oct 14, 2023

What about removing the Dispose method and have a global explicit method such as ebiten.DestroyImage (Free, Release, Collect are also potential keywords)
As for the behavior, it could be:
a. Take an **ebiten.Image (double pointer) as argument to ensure setting to nil the user-owned object after clearing internal resources (so that if fails on user side properly when accessing it again), but it probably feels a bit weird? => Would handle subimage, since it could be handled as a noop internally
b. Set the internal *ebiten.Image.image (internal image wrapped by an ebiten.Image) to nil so that any new attempt at a draw operation using it would segv => handles subimage as well

internal/atlas state is recreated when the image is used again.

The re-creation of the atlas in such cases sounds a bit complicated, no? Considering this is understandable for 2
But for 1, I don't understand why it should be possible to re-use an image as-is after disposing it (even if it's "possible")

@hajimehoshi
Copy link
Owner Author

This issue is to simplified the model of an image by removing the disposed state. I'm afraid I don't think your proposal aligns with this.

The re-creation of the atlas in such cases sounds a bit complicated, no?

An image just after NewImage is in the same situation, so there is nothing completed.

@hajimehoshi
Copy link
Owner Author

hajimehoshi commented Oct 14, 2023

I'm adding comments from Discord:

My aim is to avoid a kind of 'dangling pointer': Now a disposed image is not available and cuases panics when used as a destination. In a new model, this would never happen as an unmapped image can be treated as a new cleared image.

Actually Dispose was an necessary evil as in an the ideal world GC should resolve everything, but in the actual world it does not.

Unmap would be useful when you are sure that the image is no longer used and want to remove the internal images to save GPU memory. Sometimes image references unexpectedly live so always depending on GC is risky especially for big projects.
Actually quasilyte had a similar issue. They found unepxected references, but before resolving them, Dispose helped them to confirm to save GPU memory.

@hajimehoshi hajimehoshi changed the title ebiten: removing the 'being disposed' state of an image ebiten: add a new API to remove the 'being disposed' state of an image and deprecate Dispose Oct 17, 2023
@hajimehoshi
Copy link
Owner Author

Another name candidate is (*Image).ClearAndUnmap

@hajimehoshi hajimehoshi changed the title ebiten: add a new API to remove the 'being disposed' state of an image and deprecate Dispose ebiten: add a new image API to 'unmap' its internal state and deprecate Dispose Oct 31, 2023
@bjorndm
Copy link

bjorndm commented Nov 2, 2023

I think Image.Dispose() is not really a problem, not more than the standard library File.Close(), or close(channel) is. We can't use closed files or closed channels either. By the way, I fee Close() is a good name in stead of Dispose().

@hajimehoshi
Copy link
Owner Author

I think Image.Dispose() is not really a problem, not more than the standard library File.Close(), or close(channel) is. We can't use closed files or closed channels either. By the way, I fee Close() is a good name in stead of Dispose().

This is not a name issue. Please read #2808 (comment)

@hajimehoshi hajimehoshi changed the title ebiten: add a new image API to 'unmap' its internal state and deprecate Dispose ebiten: add a new image API to 'deallocate' its internal state and deprecate Dispose Nov 3, 2023
hajimehoshi added a commit that referenced this issue Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants