Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#4251 closed defect (fixed)

[PATCH] Paired techs don't work with the multiple commands handling

Reported by: Itms Owned by: Imarok
Priority: Release Blocker Milestone: Alpha 21
Component: UI & Simulation Keywords: patch
Cc: Patch:

Description (last modified by fatherbushido)

Reported here: https://wildfiregames.com/forum/index.php?/topic/21145-latest-commit-breaks-paired-techs/

In 0ad without mods, there is the paired techs for walls and Seleucids have also paired techs in fortress, if you want to test that.

Attachments (4)

4251_fix.2.patch (2.3 KB ) - added by Imarok 5 years ago.
Same as the paste, but replaced JSON.stringify with uneval and change warn to error
4251_fix.patch (2.3 KB ) - added by Imarok 5 years ago.
delete
4251_fix_v3.patch (2.6 KB ) - added by elexis 5 years ago.
Patch by Imarok.
4251_fix_v4.patch (2.6 KB ) - added by elexis 5 years ago.
Some newlines to keep it readable and group terms logically.

Download all attachments as: .zip

Change History (14)

comment:1 by elexis, 5 years ago

Priority: Should HaveMust Have

A second non-trivial bug found by fatherbushido:

  1. Build 2 temples (or any other building that can research techs)
  2. Produce a the mauryan healer hero (which received a research tech speed bonus when garrisoned in r18233 in wiki:Alpha21)
  3. Garrison it in one of the two buildings
  4. Select the first building, hold shift and add the second building to the selection. Notice the techs will be listed twice (as they are researched with different speeds).
  5. Select the second building, then hold shift and add the first building to the selection. Notice one of the two variants shows the tech with the same research speed, and one variant shows the techs with the different speeds.

comment:2 by Imarok, 5 years ago

Keywords: patch review added
Summary: Paired techs don't work with the multiple commands handling[PATCH] Paired techs don't work with the multiple commands handling

Fixed initial bug: http://pastebin.com/1TbwFYad (github mirror is not updated, and I'm not sure how to produce an svn patch)

comment:3 by Imarok, 5 years ago

fix for the second bug: http://pastebin.com/tZzDZie6

comment:4 by Imarok, 5 years ago

fix paired techs appear multiple (see second post in forum thread): http://pastebin.com/nzVKLHBJ (this also contains the additional two fixes)

comment:5 by fatherbushido, 5 years ago

Description: modified (diff)
Priority: Must HaveRelease Blocker

by Imarok, 5 years ago

Attachment: 4251_fix.2.patch added

Same as the paste, but replaced JSON.stringify with uneval and change warn to error

by Imarok, 5 years ago

Attachment: 4251_fix.patch added

delete

comment:6 by elexis, 5 years ago

With regards to the patch above, after the release we should add Object.Freeze calls to the cache, to ensure we get an error if someone tries to modify those objects.

Lets not forget attachment:upgradev3.patch:ticket:3823 (since noone looks at closed tickets).

comment:7 by elexis, 5 years ago

In 18788:

Don't overwrite cached template data by by cloning the referenced object, thus fix a GUI bug revealed by refs #3823. Patch by Imarok, refs #4251.

by elexis, 5 years ago

Attachment: 4251_fix_v3.patch added

Patch by Imarok.

by elexis, 5 years ago

Attachment: 4251_fix_v4.patch added

Some newlines to keep it readable and group terms logically.

comment:8 by elexis, 5 years ago

Resolution: fixed
Status: newclosed

In 18790:

Fix paired techs following r18773. Patch by Imarok, fixes #4251, refs #3823.

comment:9 by elexis, 5 years ago

Keywords: review removed

Thanks for picking up the bugreports and delivering the fixes :)

comment:10 by elexis, 5 years ago

Created #4257 for the freezing part, which should be done in alpha 22 as it can reveal random errors, as demonstrated by #3647.

Note: See TracTickets for help on using tickets.