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

Port amethyst_animation to Legion #2549

Merged
merged 24 commits into from
Jan 6, 2021
Merged

Conversation

ezpuzz
Copy link
Contributor

@ezpuzz ezpuzz commented Dec 17, 2020

Description

Continuing work from #2351 to get amethyst_animation working with legion but I've hit an impasse and my legion skills aren't developed enough yet to pick a best path forward. The AnimationControlSystem wants to pass a mutable world reference through the process_animation_control chain but that makes borrow checker unhappy. Will need to either pass Vec<(Entity, EntryMut)> in place of hierarchy or something else. Any suggestions? Edit: just used a world split and seems alright.

@ezpuzz
Copy link
Contributor Author

ezpuzz commented Dec 17, 2020

vertex skinning and examples and book not fixed yet

@ezpuzz ezpuzz added the Legion Legion related issues. label Dec 17, 2020
@ezpuzz ezpuzz mentioned this pull request Dec 17, 2020
@ezpuzz ezpuzz mentioned this pull request Jan 2, 2021
23 tasks
@ezpuzz ezpuzz changed the base branch from legion_v2 to master January 2, 2021 16:29
@ezpuzz ezpuzz self-assigned this Jan 2, 2021
@ezpuzz ezpuzz added this to In progress in Legion integration via automation Jan 2, 2021
@ezpuzz ezpuzz added this to the 0.16.0 - Legion milestone Jan 2, 2021
@ezpuzz ezpuzz linked an issue Jan 2, 2021 that may be closed by this pull request
23 tasks
@ezpuzz ezpuzz removed a link to an issue Jan 2, 2021
23 tasks
@ezpuzz ezpuzz marked this pull request as ready for review January 3, 2021 00:20
@ezpuzz
Copy link
Contributor Author

ezpuzz commented Jan 3, 2021

sprite_animation works now in a code-first way and not prefabs. we are ironing out the last bit of how prefabs should work still.

Copy link
Contributor

@valkum valkum left a comment

Choose a reason for hiding this comment

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

Overall nice work.

use uuid::Uuid;

impl TypeUuid for Sampler<MaterialPrimitive> {
const UUID: type_uuid::Bytes =
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we hide those behind a macro?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately TypeUuid doesn't support generic types yet, so this is a workaround: randomPoison/type-uuid#4

"vertex_skinning_system",
self.dep,
);
// FIXME: builder.add_system(VertexSkinningSystem);
Copy link
Contributor

Choose a reason for hiding this comment

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

What is missing for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think i've converted it now, but we won't be able to test until the gltf example is working.

@@ -42,15 +33,11 @@ pub trait AnimationSampling: Send + Sync + 'static + for<'b> ApplyData<'b> {
&mut self,
channel: &Self::Channel,
data: &Self::Primitive,
extra: &<Self as ApplyData<'a>>::ApplyData,
buffer: &mut CommandBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enforce the same naming schema for all CommandBuffers in fn arguments inside of amethyst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only see one instance of it being named differently, in the removal util. I think it would be good to stay consistent but I don't know of a linting tool to enforce it. Do you have something in mind?

amethyst_animation/src/skinning/systems.rs Outdated Show resolved Hide resolved
amethyst_animation/src/systems/control.rs Outdated Show resolved Hide resolved
amethyst_animation/src/systems/control.rs Outdated Show resolved Hide resolved
Comment on lines 60 to 111

{
let loader = resources
.get_mut::<DefaultLoader>()
.expect("Missing loader");
// let bat_prefab = loader.load("prefab/sprite_animation.ron");
// let arrow_test_prefab = loader.load("prefab/sprite_animation_test.ron");

let texture = loader.load("texture/bat.32x32.png");
let sprites = loader.load("sprites/bat.ron");
let sprite_sheet_store = resources.get::<ProcessingQueue<SpriteSheet>>().unwrap();
let sheet: Handle<SpriteSheet> =
loader.load_from_data(SpriteSheet { texture, sprites }, (), &sprite_sheet_store);

//let anims: Handle<Sampler<SpriteRenderPrimitive>> = loader.load("anims/bat.ron");
let anims = loader.load_from_data(
Sampler::<SpriteRenderPrimitive> {
input: vec![0.0, 0.2, 0.4, 0.6, 0.8, 1.0],
output: vec![
SpriteRenderPrimitive::SpriteIndex(4),
SpriteRenderPrimitive::SpriteIndex(3),
SpriteRenderPrimitive::SpriteIndex(2),
SpriteRenderPrimitive::SpriteIndex(1),
SpriteRenderPrimitive::SpriteIndex(0),
],
function: InterpolationFunction::Step,
},
(),
&resources
.get::<ProcessingQueue<Sampler<SpriteRenderPrimitive>>>()
.unwrap(),
);
let mut anim_set = AnimationSet::new();
let anim_handle: Handle<Animation<SpriteRender>> = loader.load_from_data(
Animation::<SpriteRender>::new_single(
0,
amethyst_animation::SpriteRenderChannel::SpriteIndex,
anims,
),
self.progress_counter.as_mut().unwrap(),
)
});
&resources
.get::<ProcessingQueue<Animation<SpriteRender>>>()
.unwrap(),
);
anim_set.insert(AnimationId::Fly, anim_handle);

world.push((SpriteRender::new(sheet, 0), Transform::default(), anim_set));
}
// Creates new entities with components from MyPrefabData
world.create_entity().with(bat_prefab).build();
world.create_entity().with(arrow_test_prefab).build();
// world.create_entity().with(bat_prefab).build();
// world.create_entity().with(arrow_test_prefab).build();

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this ok for now. Do we want to revert his when asset loading is fully working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm working on prefabs later, plus there is prior conversation throughout the issues of making the examples rely less on prefabs. The fastest way to get there is to port examples away from prefabs and possibly convert them back later. We should rely on user feedback as to whether or not they need prefabs for a given example.

This example works in this state to demonstrate an animated sprite so I'm happy with it as is. I think examples like this one should be focused on demonstrating the programmatic API rather than the magic of prefabs.

I have some work in progress towards a big prefab example that demos a bunch of different prefabs and swapping between them. With the legion-prefab approach you are essentially just serializing the component directly.

let (width, height) = {
let dim = world.read_resource::<ScreenDimensions>();
let dim = resources.get::<ScreenDimensions>().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably use an except here. Helps new users if they copy code from the examples.

examples/sprite_animation/main.rs Show resolved Hide resolved
@ezpuzz ezpuzz added type: improvement An improvement or change to an existing feature. type: feature A request for a new feature. type: breaking API breaking change, will require users to change their code. labels Jan 6, 2021
@ezpuzz ezpuzz changed the title [LEGION] animation port Port amethyst_animation to Legion Jan 6, 2021
@ezpuzz ezpuzz changed the title Port amethyst_animation to Legion Port amethyst_animation to Legion Jan 6, 2021
@ezpuzz
Copy link
Contributor Author

ezpuzz commented Jan 6, 2021

this is blocking gltf, animation example, and my debug-lines example fix so I am going to merge now that i've fixed @valkum's concerns

@ezpuzz ezpuzz merged commit 10cb924 into amethyst:master Jan 6, 2021
Legion integration automation moved this from In progress to Done Jan 6, 2021
@ezpuzz ezpuzz deleted the animation-legion branch January 6, 2021 16:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Legion Legion related issues. type: breaking API breaking change, will require users to change their code. type: feature A request for a new feature. type: improvement An improvement or change to an existing feature.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants