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

feat(AnimatedSprite): add AnimatedSprite #331

Merged
merged 54 commits into from
Apr 10, 2024
Merged

Conversation

andretchen0
Copy link
Contributor

@andretchen0 andretchen0 commented Jan 11, 2024

Docs: pnpm run docs:dev and then => http://localhost:5173/guide/abstractions/animated-sprite.html
Playground: pnpm run playground and then => http://localhost:5173/abstractions/animated-sprite


@JaimeTorrealba , @alvarosabu

I wonder if you guys would take a look at <AnimatedSprite /> while it's still a draft and give me some feedback.

What do you think of the API?

I'd like to know if you see any improvements to make to the API. I followed most of what Drei did, but included a few extra things that I picked up from using Deepnight's gameBase. Maybe those changes aren't necessary here – What do you think?

In particular, these props:

  • anchor – this allows differently sized images to be used in an atlas
  • definitions – this allows users to specify how each animation plays: frame order and delays. I think it's really handy for tweaking animations iteratively, but it does add about 250 lines of code, mostly for the definition parser.
  • animation – named animations are pulled from source image names. See guide/abstractions/animated-sprite.html#animation . I found this really handy when working with the gamebase, but maybe our users won't see the point.
  • all callbacks, e.g., onFrame – Drei makes a new object and sends it as an argument to the callbacks. The object includes the frame name and the whole frame – e.g., the height and width of the original sprite image. I find the frame is pretty useless for the end user, so I just provide the frame name. (I don't retain the original frame info at all. I convert the values to uv coordinates, so that doesn't have to be done every tick.) Any thoughts about that?

Do we want to include a spritesheet compiler?

A while ago, I wrote a very dumb node.js command line app for compiling spritesheets/atlas JSON from source images. I could rewrite it for the browser and include it in the docs. That would let users compile their own spritesheets/atlas JSON online with a drag and drop interface. But do we want to support that going forward?

TexturePacker is paid software. For context, Drei and our <AnimatedSprite /> read simple TexturePacker atlas JSON.
But Drei and our <AnimatedSprite /> don't/won't support most of the advanced features of TexturePacker.

Copy link

stackblitz bot commented Jan 11, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

netlify bot commented Jan 11, 2024

Deploy Preview for cientos-tresjs ready!

Name Link
🔨 Latest commit 574e3bd
🔍 Latest deploy log https://app.netlify.com/sites/cientos-tresjs/deploys/65bfe2103b1ecc0008e63d86
😎 Deploy Preview https://deploy-preview-331--cientos-tresjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JaimeTorrealba
Copy link
Member

Hi @andretchen0 this is a nice work, thank you so much. To be honest regarding code, almost nothing. I would like to know something.

  1. If I have a spritesheet, and I would like to use only the first row, can I do that here? Without all the complicated process of the atlas, could we add that, like to be able to cut the sprite?
    I find myself using it https://lab.jaimetorrealba.com/sprites_controls_demos

Evil Druid

Cientos Playground - AnimatedSprite

  1. Should the asSprite option as true by default? (what do you think?)

  2. I'll put the "Compiling an atlas" section almost at the top, I believe there are people who are not familiar with it (like me) and as is "almost" required, should be good to adding right after the "usage"

  3. Non related, but did you remember how we removed the "multiple instances of ThreeJs" that blocks OrbitControls to move freely?

image

Copy link
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know is a draft, a look really nice, I want to use this one!

/>
```

### Ranges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I get confused, I tough ranges was a prop maybe to put, animations as range

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha. Thanks for the feedback. It's a good catch. I'll clear that up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it clearer now?

(Fwiw, the range is supposed to be given as the animation prop. Just in case that's still not clear.)


A `[number, number]` range can be supplied as the `animation` prop. The numbers correspond to the position of the frame in the `atlas` `frames` array, starting with `0`. The first `number` in the range represents the start frame of the animation. The last `number` represents the end frame.

### Single frame
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@andretchen0
Copy link
Contributor Author

@JaimeTorrealba Thanks for the feedback!

@andretchen0
Copy link
Contributor Author

If I have a spritesheet, and I would like to use only the first row, can I do that here? Without all the complicated process of the atlas, could we add that, like to be able to cut the sprite?
I find myself using it https://lab.jaimetorrealba.com/sprites_controls_demos

We can do something different, but I copied the Drei setup here. Afaik, without an atlas, the Drei version only allows for a single row of images — with all the sprites being the same size, evenly spaced, and e.g., no padding on the spritesheet itself.

We could take [number, number] on the atlas prop to mean number of columns and number of rows. Again with all sprites being the same size, evenly spaced, and no padding on the spritesheet itself.

That would work for the first row of your example spritesheet, I think. And that's an ok change in my book as far as I can see at the moment.

Wherever we do, unfortunately spritesheets in the wild tend to be complied irregularly, and often distributed without an atlas. We could make an atlas builder tool that lets people divide up an already complied spritesheet, but I don't think we'd want to support that in the long term.

Probably the best thing to do for irregular spritesheets without an atlas is to type out an atlas. But I understand that's not ideal for a lot of users.

Open to additional ideas. But I'll go ahead and add row/cols like above unless there's an unforeseen problem.

@andretchen0
Copy link
Contributor Author

andretchen0 commented Jan 16, 2024

asSprite as default

I'd be ok with that.

I defaulted to false because sprites are a bit hard to use in relation to other 3D elements in the scene. They always face the camera.

The current implementation of <AnimatedSprite /> also makes working with sprites a bit wonky if:

  • the frames are different sizes
  • the camera is not looking straight down the z
  • and the anchor is not the default

Maybe this can be worked around though.

Let me know what you think.

@JaimeTorrealba
Copy link
Member

Exactly we don't want to support another library (still if there is another free good option, we can send the user there, having a warning that is not under our scope).
I don't pretend to add full support to non-atlas, as you say they tend to be irregulars and could be a big headhache for us. My intention here is just the control with one row (this pages come with a lot of sprites, and I normally use just one row: https://www.spriters-resource.com/)
If the people want something more complex, they need to use atlases

Regarded asSprite as default I would expect to behave like sprite materials (but I could be wrong, maybe let's ask to someone else)

@andretchen0
Copy link
Contributor Author

andretchen0 commented Jan 16, 2024

My intention here is just the control with one row (this pages come with a lot of sprites, and I normally use just one row: https://www.spriters-resource.com/) If the people want something more complex, they need to use atlases

Gotcha.

Here's the only way I know that will work for spriters resource rips:

  • split the sheets into individual images in e.g., GIMP
  • stack the images in layers so they line up for animation
  • export layers as individual images
  • compile individual images to a spritesheet/atlas

At least, that's the only solution that I see.


@JaimeTorrealba the rest of this is just details about the problem. Feel free to skip. I'm just including it because I don't want you to feel like I'm dismissing your use case.

I've banged my head against the wall with spritesheets from spriters resource before. I think they're mostly compiled by hand from screenshots or other means. As far as I can tell, their purpose is to distribute art to fans, not to make it easy to animate – because they don't include information for animation, i.e., how individual frames line up.

Normally, some care has to be taken to get the animation frames to line up. E.g., here's Wesnoth's documentation for sprite creators.

  • Units are centered horizontally
  • Unit's feet are positioned around 55 pixel down from the top edge of the canvas, lower for taller units if necessary

https://wiki.wesnoth.org/Creating_Unit_Art#Basic_unit_image_specifications

That gets lost in spriters resource rips. Without that information, there's no way to automate the animation, as far as I know.

I'd love to be wrong though – it'd be great for our users. So if you see a way to use spriters resource spritesheets without recompiling the spritesheet/atlas, don't hesitate to tell me. That would be a welcome thing for me to learn!

Copy link
Member

@JaimeTorrealba JaimeTorrealba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, I made some comments, all non-blocking

  1. Why is this PR in draft? What else is missing?

@andretchen0
Copy link
Contributor Author

@JaimeTorrealba

1. Why is this PR in draft? What else is missing?

Is it showing up as a draft for you? I see it as an open PR:

Screenshot 2024-02-04 at 20 08 42

@andretchen0 andretchen0 mentioned this pull request Feb 7, 2024
Closed
16 tasks
@andretchen0
Copy link
Contributor Author

@alvarosabu @JaimeTorrealba

Do we want to merge in v3 or wait for v4?

There are no conflicts with v3, but if we wait for v4, <TresSprite /> should be clickable so we can add that back in.

@JaimeTorrealba
Copy link
Member

I thought this one was for the v4 (but depend on you, my friend)

Alvaro is coming the 19th. We have time since v4 comes in the second Q approx.

If you decide to merge for the v4 remember to point to the v4 branch and add the tag :)

@andretchen0
Copy link
Contributor Author

I thought this one was for the v4 (but depend on you, my friend)

Alvaro is coming the 19th. We have time since v4 comes in the second Q approx.

If you decide to merge for the v4 remember to point to the v4 branch and add the tag :)

Thanks for the pointers! 😀 And thanks for the PR approval.

I'll wait for v4 then. That'll give us one more thing to announce.

@JaimeTorrealba JaimeTorrealba added v4 p2-nice-to-have Not breaking anything but nice to have (priority) labels Apr 5, 2024
@JaimeTorrealba
Copy link
Member

Hello @andretchen0, please remember to change the target branch to v4.
We can merge it now

@andretchen0 andretchen0 changed the base branch from main to v4 April 7, 2024 16:51
@andretchen0
Copy link
Contributor Author

Hey @JaimeTorrealba ,

I updated AnimatedSprite for v4. Could you please have a look?

Changes to pay attention to are in the recent commit titles. But namely:

  • removed the <Suspense /> from the component, it's up to users to add it
  • added :as-sprite prop, default is true
  • removed the @click from the component – it should be handled by the new event system, but will need to check against new event system when it's ready.

Thanks!

@andretchen0 andretchen0 linked an issue Apr 7, 2024 that may be closed by this pull request
4 tasks
@alvarosabu
Copy link
Member

Hey there @andretchen0 @JaimeTorrealba is this ready to merge?

@andretchen0 andretchen0 merged commit 63170ec into v4 Apr 10, 2024
3 checks passed
@andretchen0
Copy link
Contributor Author

@alvarosabu

Hey there @andretchen0 @JaimeTorrealba is this ready to merge?

Merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority) v4
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Sprite component
3 participants