patchBattle for Wesnoth - Patches: patch #3846, Fixed bug #20734: Default 100 gold...

 
 
Show feedback again

patch #3846: Fixed bug #20734: Default 100 gold is applied when gold isn't specified in scenario.cfg

Submitted by:  Mateusz Kołaczek <pl_kolek>
Submitted on:  Fri 12 Apr 2013 12:08:30 PM UTC  
 
Priority: 5 - NormalStatus: In Progress
Privacy: PublicAssigned to: Iurii Chernyi <crab>
Open/Closed: Open

Add a New Comment (Rich MarkupRich Markup):
   

You are not logged in

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

 

(Jump to the original submission Jump to the original submission)

Mon 13 May 2013 03:38:14 PM UTC, comment #7:

It is the leader's id, not the side one's. Sides don't normally have an id, unless the wml author explicitely specifies a save_id. We could make specifying a non-empty save_id for persistent sides mandatory. If it's not given, stderr message and exit. wmllint could also check it for existance. (For my own campaign I decided that it's mandatory long ago.) This could potentially avoid bugs such as http://forums.wesnoth.org/viewtopic.php?f=6&t=38774&

I recall someone adding [side][leader] but it's not documented in the wiki.

Anonymissimus <anonymissimus>
Project Member
Fri 10 May 2013 05:53:56 PM UTC, comment #6:

No, I didn't notice that. From what Crab_ said, I understood that patch such as the first of mine (and probably ayne's patch?) is duplication of the carryover code to other places, and it should be handled just there to avoid maintenance problems.

Judging by the code, I think that we already rely on one of those tags being non-empty. For example:
-transfer_all_to (gamestatus.cpp) checks save_id or id
-teaminfo::read does the same

However, if there are so many ways to just set the side id, then what would you suggest instead?

Mateusz Kołaczek <pl_kolek>
Fri 10 May 2013 04:01:03 PM UTC, comment #5:

Did you notice that meanwhile ayne committed a different fix, probably without knowing about this ? (commit 0d2eeeeeefaa8fa87d2861d0ed1011574fda5495)

I think one can't rely on at least one of [side]save_id= or [side]id= not being empty. The leader could also be given by [side][leader]. Perhaps even [side][unit] would work. Or not giving the leader a manual id at all (which is bad of course, since you then cannot reference it).

Anonymissimus <anonymissimus>
Project Member
Thu 09 May 2013 05:39:46 PM UTC, comment #4:

Correct version in attachment now.

(file #17941)

Mateusz Kołaczek <pl_kolek>
Tue 07 May 2013 07:13:46 PM UTC, comment #3:

The carryover code was not called, because the sides were not initialised in the beginning of the play_campaign function. I created a loop that adds the teams not present.

Patch attached

(file #17918)

Mateusz Kołaczek <pl_kolek>
Fri 03 May 2013 05:53:41 AM UTC, comment #2:

Thanks for the patch, but I'd prefer this fixed in a different way. Carryover code should be responsible for setting the gold to default value. That code is located in gamestatus.cpp, and it has the 'set gold to default value' logic. The problem is that this carryover-to-next-scenario code is NOT called before first scenario, but, I'd say that it should be called.

Iurii Chernyi <crab>
Project MemberIn charge of this item.
Fri 12 Apr 2013 02:24:16 PM UTC, comment #1:

"I also added gold=100 for SoF:1 for clarity."
I rather wouldn't want that (from the UMC author point of view). Using conventions in mainline tends to outsource workload onto UMC authors, since their addons have the more unusual wml then. Only the fact that this wasn't there made it possible to discover and reproduce the bug with a mainline scenario.

Anonymissimus <anonymissimus>
Project Member
Fri 12 Apr 2013 12:08:30 PM UTC, original submission:

Both gold and start_gold are set to default 100 if nothing is specified in config file. Previously only start_gold was being set, which is used only in displaying starting gold in status table.

I also added gold=100 for SoF:1 for clarity.

Mateusz Kołaczek <pl_kolek>

 

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

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by crab (Posted a comment)
  • -unavailable- added by anonymissimus (Posted a comment)
  • -unavailable- added by pl_kolek (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 5 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 09 May 2013 05:39:46 PM UTCpl_kolekAttached File-=>Added 0001-Fixed-default-100-gold-not-applied-on-the-campaign-s.patch, #17941
    Tue 07 May 2013 07:13:46 PM UTCpl_kolekAttached File-=>Added 0001-Fixed-default-100-gold-not-applied-on-the-campaign-s.patch, #17918
    Fri 03 May 2013 05:53:41 AM UTCcrabAssigned toNone=>crab
    Fri 03 May 2013 05:53:40 AM UTCcrabStatusNone=>In Progress
    Fri 12 Apr 2013 12:08:30 PM UTCpl_kolekAttached File-=>Added 0001-Fixed-bug-20734-gold-is-set-to-default-if-it-s-not-p.patch, #17740
    Show feedback again

    Back to the top


    Powered by Savane 3.1-cleanup