patchBattle for Wesnoth - Patches: patch #3862, Fixed bug #20704: using...

Show feedback again

patch #3862: Fixed bug #20704: using 'apply_to=new_animation' with 'id'

Submitted by:  Andrius Silinskas <thunderstruck>
Submitted on:  Sun Apr 21 21:37:42 2013  
Priority: 5 - NormalStatus: Wont Do
Privacy: PublicAssigned to: None
Open/Closed: Closed

Add a New Comment (Rich MarkupRich Markup):

You are not logged in

Please log in, so followups can be emailed to you.


Fri Apr 26 01:19:59 2013, comment #4:

OK, I'll close this.

Yes, the carryover code is where I was looking. I did think it would have been nice to fix both ends, but there are a lot of places in unit.cpp that assume or assert that the resources:: are available. And that is normally a sound assumption, so it is probably not worth providing alternative functionality when it fails. (Another way to fix this in unit.cpp would be to check if resources::controller is NULL. If it is, just add the animation, otherwise use the cache.) I guess some documentation should be made to that effect.

J Tyne <jamit>
Project Member
Wed Apr 24 06:39:05 2013, comment #3:

It seems that I've made a false assumption on animation cache. Thanks jamit for spotting it.

Could this patch be closed/cancelled then?

The other solution to this problem would be to change carryover code where new units are created after the game. I think that jamit was working on that. Or maybe it would be better to move animation_cache somewhere else.

Andrius Silinskas <thunderstruck>
Project Member
Wed Apr 24 05:35:43 2013, comment #2:

Correction to the preceding (thunderstruck's assessment and my tacit acceptance of that): the cache is implemented. The cache is populated because the "built" variable (removed by this patch) is a reference. It references an element in the cache, so when animations are added to built, they are added to the cache.

J Tyne <jamit>
Project Member
Wed Apr 24 03:34:30 2013, comment #1:

You might want to run this by boucman, as he added the cache in r46088 to deal with bug #16363. In particular, I notice that that bug was never confirmed fixed, so boucman might not realize he neglected to add the code that would populate the cache. If that bug still warrants being open, this might be a not good thing to get rid of (meaning a different fix to this part of the code might be in order).

J Tyne <jamit>
Project Member
Sun Apr 21 21:37:42 2013, original submission:

The bug was caused by accessing resources::controller after it was destroyed. This was done for performance reasons to enable re-use of animations with a help of animation_cache. But it seems that animation_cache was never implemented.

The solution for the bug was simply to remove the code which caused the problem. References to animation_cache have been removed as well since it is not used anywhere.

Some arguments from IRC:
<lipkabb> It has an almost completely unnecessary dependance on the controller.
<thunderstruck> lipkabb, that's what I think as well.
<lipkabb> Removing that dependency would make the code more robust, even if a bit slower. is going to be updated after the patch is accepted.

Andrius Silinskas <thunderstruck>
Project Member


(Note: upload size limit is set to 1024 kB, after insertion of the required escape characters.)

Attach File(s):

Attached Files


Depends on the following items: None found

Items that depend on this one: None found


Carbon-Copy List
  • -unavailable- added by jamit (Posted a comment)
  • -unavailable- added by thunderstruck (Submitted the item)

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.


    Error: not logged in



    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri Apr 26 01:19:59 2013jamitStatusNone=>Wont Do
    Sun Apr 21 21:37:42 2013thunderstruckAttached File-=>Added new_animation_id.patch, #17793
    Show feedback again

    Back to the top

    Powered by Savane 3.1-cleanup