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

Expose layers to public API #3

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dkotik
Copy link

@dkotik dkotik commented Dec 19, 2023

I tried to make the least amount of changes to get to the functionality I wanted. The API can be massaged more later. I also noticed that tests are little bit disorganized - I can submit another PR later to help with that.

@dkotik dkotik mentioned this pull request Dec 19, 2023
@askeladdk
Copy link
Owner

Hi, thanks for the PR. It's a good start but I have some comments / suggestions.

  1. I don't think it's neccessary to expose all the buildX() methods. Those are really only meant to be used internally.

  2. You have exposed the Layer struct but there is no way to turn it into an image.Image. I don't that struct needs to be exposed directly.

What we want to have is the ability to produce either a single flattened image atlas or a slice of unflattened image atlases. I think we can approach it as follows:

  1. Add a new field Layers []image.Image to Asesprite.

  2. Add a new method Flatten() *Aseprite to File to produce the current Aseprite with the Layers field left nil. (it just does the same thing as what func (spr *Aseprite) readFrom() does now)

  3. Add a new method Layered() *Aseprite to File to product an Aseprite where Image is nil but Layers contains all the unflattened layers. Those layers do also need to be image atlases so that the frame data can be used with them.

  4. (Optional) Add a new method Flatten() to Aseprite to flatten a layered Aseprite into a single image, as a convenience.

What do you think?

@dkotik
Copy link
Author

dkotik commented Dec 26, 2023

Dear friend,

I read your recommendation with great attention for about 45 minutes and studied the code more. I submitted two more commits. The first to make build methods private as you requested. The second to drop (spr *Aseprite) readFrom method in favor a New method that takes the raw decoded format.

My reasoning is in line with my initial suggestion to split the construction of Aseprite into two: first get raw File, then Aseprite out of it, so that layers could be manipulated in some way before the image is flattened.

I don't think that adding []Layers field would be better for two reasons:

  1. It complicates the final object.
  2. The blending modes of separate atlases becomes a mystery, unless somehow lifted to the Aseprite level. It will work, but it may not be intuitive to the consumer of the library.

With my current approach, if layers need to be manipulated (in my case, only filtered) the usage becomes:

 // normal way, no layer filtering
spr, _ := Read(r)

 // new way, with layer filtering
f, _ := NewFile(r)
f.FilterLayers(filterFunc)
spr := New(f)

Other types of manipulations could be added in the future. I think your suggestion can certainly work, but it will take more lines to implement and potentially produce a less flexible API surface for later modification. What do you think?

@dkotik
Copy link
Author

dkotik commented Dec 28, 2023

I also suggest changing *File.FilterLayers signature to return a new *File instead of modifying the current one, so something like this can be composed more naturally:

f, _ := NewFile(r)
spr := New(f.FilterLayers(filterFunc))
differentLayers := New(f.FilterLayers(differentFilterFunc))

I will submit the change after I hear some feedback from you.

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.

2 participants