Opened 10 years ago

Closed 8 years ago

Last modified 7 years ago

#2324 closed enhancement (fixed)

[PATCH] Actor format update

Reported by: wraitii Owned by: wraitii
Priority: Should Have Milestone: Alpha 21
Component: Core engine Keywords: patch
Cc: philip Patch:

Description

I've been working on a change in the actor format in order to support a few more functions that could be useful. The code works afaik, I'm just posting because the style isn't necessarily very nice (lots of repetitions), an I'd like some feedback before making changes. Also, actor editor is unsupported still.

It changes nothing except for animations blocks (see below) and the "version" which is now 2, as older format is supported (code repetition again).

<animations>
	<Idle>
		<animation file="biped/cavalryidle.psa" frequency="20" speed="50"/>
		<animation file="biped/cavalryidle2.psa" frequency="30" speed="50"/>
		<animation file="biped/cavalryidle3.psa" name="horse_rearing" speed="50"/>
		<animation file="biped/cavalryidle4.psa" frequency="10" name="horse_lower_head" speed="50"/>
		<animation file="biped/cavalryidle5.psa" frequency="15" name="horse_lower_head" speed="50"/>
	</Idle>
	<Walk><animation file="biped/cavalryidle.psa" name="Walk" speed="50"/></Walk>
	<Run><animation file="biped/rider_gallop.psa" name="Run" speed="40"/></Run>
	<attack_ranged><animation event="0.59" file="biped/rider_javelin_atk_a.psa" load="0" name="attack_ranged" speed="50"/></attack_ranged>
	<attack_slaughter><animation event="0.5" file="cavalry/sword/attack/rider_sword_shield_atk_a.psa" name="attack_slaughter" speed="400"/></attack_slaughter>
	<gather_meat><animation event="0.5" file="biped/rider_spear_shield_atk_a.psa" name="gather_meat" speed="300"/></gather_meat>
	<death><animation file="biped/rider_sword_death_a.psa" name="death" speed="230"/></death>
</animations>

So why the change? First of all this allows for "frequency", which makes it a lot easier to finetune how often animation will be chosen (we would not need the balise change for that, but it makes it nicer imo). Note that default frequency is "0".
Secondly, and as a less important but nonetheless good improvement, this allows to make sure models and their props remain synchronized (using the same "name" for both the props and the parent model). Why is that good? Let's take an example: you have a horse mesh with material colour_trans.xml. You want the mane to have transparency, so you want it to use basic_trans.xml. To do this, you have to make the mane a prop of the horse. Now say the horse has two idle animations, one where it turns its head left, one where it turns its head right. The mane also has those two animations. But how can you be sure that the right one will be chosen? You can't. Using "name", you could. So that becomes possible.
Also the "frequency" can be set to 0, but is still used if it's the only anim with a fitting name. This could be used to make actors both props and main models using clever names. Dunno if it would actually make sense, but it's good to know.

Note that for most of our props and models, this changes nothing, as you can still have several anims with the same or no name.

Attachments (12)

ActorModel.patch (38.7 KB ) - added by wraitii 10 years ago.
ActorModel.2.patch (35.9 KB ) - added by wraitii 10 years ago.
cleaned up version
actoreditorpatch.patch (6.0 KB ) - added by wraitii 10 years ago.
ActorFormat.patch (21.5 KB ) - added by wraitii 8 years ago.
ActorFormat.2.patch (21.5 KB ) - added by wraitii 8 years ago.
ActorFormat.3.patch (22.7 KB ) - added by wraitii 8 years ago.
ActorFormat.4.patch (24.6 KB ) - added by wraitii 8 years ago.
This one is RC
actorFormat_variationnames.diff (19.2 KB ) - added by sanderd17 8 years ago.
set_m_Looping_for_model_props_too.diff (652 bytes ) - added by Radagast 7 years ago.
do_not_set_m_Looping_for_model_props_consistently.diff (430 bytes ) - added by Radagast 7 years ago.
store_animname_in_lower_case_for_consistency_to_prevent_animation_or_variant_not_found.diff (487 bytes ) - added by Radagast 7 years ago.
correct_a_variant_selection_comment.diff (689 bytes ) - added by Radagast 7 years ago.

Download all attachments as: .zip

Change History (28)

by wraitii, 10 years ago

Attachment: ActorModel.patch added

by wraitii, 10 years ago

Attachment: ActorModel.2.patch added

cleaned up version

by wraitii, 10 years ago

Attachment: actoreditorpatch.patch added

comment:1 by wraitii, 10 years ago

wip atlas fix, this is broken. It segfaults weirdly where it says so in the comment code. Also it's a complete hack of everything. I'll ask for Philip's input.

comment:2 by Radagast, 10 years ago

Looks good. (Test post as server not available.)

Version 0, edited 10 years ago by Radagast (next)

comment:3 by Josh, 10 years ago

Cc: philip added
Milestone: Alpha 16Alpha 17

comment:4 by Itms, 10 years ago

Milestone: Alpha 17Alpha 18

comment:5 by leper, 9 years ago

Keywords: wip added; review removed
Milestone: Alpha 18Backlog

Removing review as there still was no push for any feedback for quite a lot of time and even the cleaned up patch isn't nice to read due to bad whitespace changes.

comment:6 by wraitii, 8 years ago

Keywords: review added; wip removed
Milestone: BacklogAlpha 20

Since Enrique has asked me if I could commit this sometimes, I've updated the patch.

Should work fine. Supports the old version of actors (in the actor editor). New version of actors isn't supported by the actor editor and would probably crash or fail weirdly. I've asked trompetin to give it a look, in the meantime we could refuse to load actors with the new version (which are not any for now).

by wraitii, 8 years ago

Attachment: ActorFormat.patch added

by wraitii, 8 years ago

Attachment: ActorFormat.2.patch added

by wraitii, 8 years ago

Attachment: ActorFormat.3.patch added

by wraitii, 8 years ago

Attachment: ActorFormat.4.patch added

This one is RC

comment:7 by Itms, 8 years ago

Milestone: Alpha 20Alpha 21

comment:8 by sanderd17, 8 years ago

I wonder if the same thing can't be achieved more simple:

  • Allow a frequency attribute on animations
  • When no animation name in the set matches the selected names, choose a random one according to the frequencies (and never pick one without frequency). With a final fallback to the Idle animations as it's now.

It would stay compatible with the current XML schema, and it would still work for the following reason:

Animations can be defined inside <variant> elements, and variants can be chosen by name. So if the selection is set to "Walk", and there's a "Walk" variant, the <animations> set of that variant will override the other animations. So the only possible animations will be the ones walking. Then you can just select one by frequency, and store the name to reuse for it's props (when appropriate).

In case you don't need a a prop to have the same animation, you don't need to give a name to your animation, but it should be clear from the variant name when the variation should be used.

So you can have a variant like this, and it will pick a random one:

<variant name="Walk">
  <animations>
    <animation file="infantry/general/walk1.psa" frequency="1" speed="250"/>
    <animation file="infantry/general/walk2.psa" frequency="1" speed="250"/>
  </animations>
</variant>

Or a variant like this, and it will save the name for the props:

<variant name="Walk">
  <animations>
    <animation file="infantry/general/walk1.psa" frequency="1" name="walk1" speed="250"/>
    <animation file="infantry/general/walk2.psa" frequency="1" name="walk2" speed="250"/>
  </animations>
</variant>

by sanderd17, 8 years ago

comment:9 by sanderd17, 8 years ago

I attached a patch, inspired by code changes of Wraitii, but that implements my view.

Explanation of the changes:

  • The modified actor is just an example of how it works, and shouldn't be committed; the walk animation now switches between regular walk and gallop. And when galloping, the rider will execute the "slaughter" animation, just to easily see the animations are synced.
  • Most of the touched files are just passing the new frequency attribute.
  • GetAnimations in ObjectEntry now guarantees to return at least one animation, and has some fallback options (first fall back to the animations that have no matched name but a frequency, then fall back to the idle animations which should always be present).
  • GetRandomAnimation in ObjectEntry now uses the frequencies to find a random animation from the GetAnimations list. If there are no frequencies defined (like most units currently), it just uses a uniform distribution.
  • SModelAnimState now doesn't save a list of animations and an animation index anymore. But just keeps a reference to the current animation, and a reference to the object, in order to use the GetRandomAnimation method from there.
  • The root actor in UnitAnimation now picks a random animation by the state name. The name of the chosen animation is saved under m_AnimationName, and then used for the props. If the root switches animation to some differently named animation, the props are also updated immediately (if animations are designed for each other, they should be equally long anyway, so not show a jump).

I don't know if it would be better to cache the list of possible animations per name. And if the references used are safe enough. But it all seems to be working nicely now.

comment:10 by sanderd17, 8 years ago

Note, update documentation after this patch. F.e. the Actors page and possibly code documentation.

comment:11 by sanderd17, 8 years ago

Resolution: fixed
Status: newclosed

In 18265:

Allow to give a frequency to animations, and allow to sync randomly selected animations on base model and prop. Fixes #2324

comment:12 by sanderd17, 8 years ago

Keywords: review removed

comment:13 by elexis, 7 years ago

Please create a new ticket with a brief problem description and add the rfc keyword, since noone looks at closed tickets or tickets without rfc/review keywords.

comment:14 by wraitii, 7 years ago

In 19031:

Fix animation syncronisation between actor props. Fixes #2324 one more time. Refs [18568] and [18265]. Reported by
wowgetoffyourcellphone

comment:15 by elexis, 7 years ago

refs #4389

comment:16 by leper, 7 years ago

Why are there so many occurences of m_ID = "" in so many places (mostly init) when not doing anything would have the same effect? And why does UpdateAnimationID need a reference if it doesn't care about it at all?

And something something reviews something.

Note: See TracTickets for help on using tickets.