Skip to content
This repository has been archived by the owner on Apr 18, 2022. It is now read-only.

feat: Add texture animation #641

Merged
merged 2 commits into from
Apr 17, 2018
Merged

feat: Add texture animation #641

merged 2 commits into from
Apr 17, 2018

Conversation

Rhuagh
Copy link
Member

@Rhuagh Rhuagh commented Apr 13, 2018 •
edited
Loading

Supports swapping textures (using indexing into a MaterialTextureSet resource) and modifying offset into a texture.
The PR also adds support for targeting parts of a texture, using a Material defined offset.

Material only support the Step interpolation function, all other functions will result in a panic (except possibly a user supplied function).

This change is Reviewable

AnneKitsune
AnneKitsune reviewed Apr 13, 2018
View reviewed changes
Copy link
Contributor

@AnneKitsune AnneKitsune left a comment

Choose a reason for hiding this comment

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

I went through quickly and it seems good. Will do a full review later (but don't wait on me to merge if everything is fine)

@azriel91 azriel91 mentioned this pull request Apr 15, 2018
Merged
@azriel91
Copy link
Member

The flat passes haven't got buffers set up for the texture offsets, and using Material animation using DrawFlat (and probably DrawFlatSeparate) gives an error:

Buffer update for effect failed! Buffer not found: "AlbedoOffset"

I got around it in #638 by calling setup_textures(&mut builder, &TEXTURES);, though I don't know where TEXTURES comes from, and whether I should've just created an "AlbedoOffset" buffer instead.

@Rhuagh
Copy link
Member Author

Rhuagh commented Apr 15, 2018

Ah, that was a miss from an earlier PR, all passes should call setup_textures, the Flat ones didn't, apparently.

@Rhuagh
Copy link
Member Author

Rhuagh commented Apr 15, 2018

Fixed now.

@Rhuagh Rhuagh mentioned this pull request Apr 15, 2018
Merged
@Rhuagh
Copy link
Member Author

Rhuagh commented Apr 16, 2018

r? @Jojolepro @Xaeroxe

Xaeroxe
Xaeroxe approved these changes Apr 17, 2018
View reviewed changes
Copy link
Member

@Xaeroxe Xaeroxe left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@AnneKitsune
Copy link
Contributor

I'll check later today if I get time

AnneKitsune
AnneKitsune approved these changes Apr 17, 2018
View reviewed changes
Copy link
Contributor

@AnneKitsune AnneKitsune left a comment

Choose a reason for hiding this comment

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

Thanks!

amethyst_animation/src/material.rs
Offset((f32, f32), (f32, f32)),
}

impl InterpolationPrimitive for MaterialPrimitive {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too sure I understand the point of adding a trait if you just panic on all functions..?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because of the trait bound on the interpolation functions in minterpolate that we use. I will implement these for some of the variants later on. For now we can only use step interpolation that won't actually call any of the functions

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

amethyst_renderer/src/pass/shaded_util.rs
@@ -101,8 +101,12 @@ pub(crate) fn set_light_args(
pub(crate) fn setup_light_buffers(builder: &mut EffectBuilder) {
builder
.with_raw_constant_buffer("FragmentArgs", mem::size_of::<FragmentArgs>(), 1)
.with_raw_constant_buffer("PointLights", mem::size_of::<PointLight>(), 128)
.with_raw_constant_buffer("DirectionalLights", mem::size_of::<DirectionalLight>(), 16)
.with_raw_constant_buffer("PointLights", mem::size_of::<PointLightPod>(), 128)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this be user defined? (out of the scope of this PR, I assume?)

Copy link
Member Author

Choose a reason for hiding this comment

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

It needs to match the size in the shader I believe

Copy link
Contributor

Choose a reason for hiding this comment

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

https://computergraphics.stackexchange.com/questions/5323/dynamic-array-in-glsl?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa
It appears that what I was thinking of doesn't work.
👍

@Xaeroxe
Copy link
Member

Xaeroxe commented Apr 17, 2018

bors r+

bors bot added a commit that referenced this pull request Apr 17, 2018
@bors @Rhuagh
Merge #641 #648
69f6084 
641: feat: Add texture animation r=Xaeroxe a=Rhuagh

Supports swapping textures (using indexing into a `MaterialTextureSet` resource) and modifying offset into a texture.
The PR also adds support for targeting parts of a texture, using a `Material` defined offset.

Material only support the `Step` interpolation function, all other functions will result in a panic (except possibly a user supplied function).

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/641)
<!-- Reviewable:end -->


648: fix: Update winit+glutin+gfx_window_* r=Xaeroxe a=Rhuagh

Fixes #544. 

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/648)
<!-- Reviewable:end -->


Co-authored-by: Simon Rönnberg <seamonr@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 17, 2018

Build succeeded

@bors bors bot merged commit a1b85d5 into amethyst:develop Apr 17, 2018
@Rhuagh Rhuagh deleted the feature/texture-animation branch April 18, 2018 18:49
bors bot added a commit that referenced this pull request Apr 22, 2018
@bors @Rhuagh
Merge #644
ba66d1d 
644: feat: Deferred start animation r=Xaeroxe,jojolepro a=Rhuagh

Depends on #641.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/644)
<!-- Reviewable:end -->


Co-authored-by: Simon Rönnberg <seamonr@gmail.com>
bors bot added a commit that referenced this pull request Apr 23, 2018
@bors @azriel91
Merge #638
7cd85dd 
638: Feature/634/sprite rendering and animation r=jojolepro,Xaeroxe a=azriel91

Attempt at sprite animation: #634. This is branched off #641.

![example](https://user-images.githubusercontent.com/2993230/39100375-798d6d82-46dd-11e8-8869-550fcb0fe887.gif)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/638)
<!-- Reviewable:end -->


Co-authored-by: Azriel Hoh <azriel91@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AnneKitsune AnneKitsune AnneKitsune approved these changes

@Xaeroxe Xaeroxe Xaeroxe approved these changes

Assignees
No one assigned
Labels
team: rendering type: feature A request for a new feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Rhuagh @azriel91 @AnneKitsune @Xaeroxe