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 21 Apr 2013 09:37:42 PM UTC  
 
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 26 Apr 2013 01:19:59 AM UTC, 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 24 Apr 2013 06:39:05 AM UTC, 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 24 Apr 2013 05:35:43 AM UTC, 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 24 Apr 2013 03:34:30 AM UTC, 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 21 Apr 2013 09:37:42 PM UTC, 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.

http://wiki.wesnoth.org/EffectWML 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):
   
   
Comment:
   

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.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Fri 26 Apr 2013 01:19:59 AM UTCjamitStatusNone=>Wont Do
      Open/ClosedOpen=>Closed
    Sun 21 Apr 2013 09:37:42 PM UTCthunderstruckAttached File-=>Added new_animation_id.patch, #17793
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup