Page MenuHomeWildfire Games

Show correct trade info for observers
ClosedPublic

Authored by bb on Sep 23 2020, 8:22 PM.

Details

Summary

Since rP23072 do not show the correct trade percentages anymore. Pre D2810 they inherited the initial gaia settingsa and kept that for the rest of time. After D2810 it shows undefined (since observers don't get a playerInterface anymore). Observers arguably should see the values for the viewedPlayer, which was broken by rP23072.

Also nuking a unused argument, looks like relic pre rP18283

Test Plan

Watch the trade values change as observer
Consider whether there is a better time to update the values (dialog open or viewedPlayer change doesn't work since the values can change during the dialog is open)

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

bb updated this revision to Diff 13523.Sep 23 2020, 8:22 PM
bb created this revision.
bb requested review of this revision.Sep 23 2020, 8:32 PM
bb added inline comments.Sep 23 2020, 9:46 PM
binaries/data/mods/public/gui/session/trade/TradeButtonManager.js
23 ↗(On Diff #13523)

Maybe this can be improved somewhat, suggestions welcome

Freagarach accepted this revision.Sep 24 2020, 7:55 AM
Freagarach added a subscriber: Freagarach.

Works as advertised.
Trading goods may be affected by triggers and will be updated accordingly now.

binaries/data/mods/public/gui/session/trade/TradeButtonManager.js
23 ↗(On Diff #13523)

Do you need the comment at all?

This revision is now accepted and ready to land.Sep 24 2020, 7:55 AM
Freagarach requested changes to this revision.Sep 24 2020, 12:56 PM

Multiplayer is weird now.

This revision now requires changes to proceed.Sep 24 2020, 12:56 PM
bb added a comment.Sep 24 2020, 6:29 PM

refs #4476

There is a problem in this patch, mostly noticeable in MP: When changing the values as a player, the values will first change by the GUI handling. Then at the next simUpdate (new turn), the values will move back to the old ones and a few turns later, the values will move back to what they should be (since the command is only executed a few turns later). This is obviously wrong. To counter this, we can add a if (g_Observer) check. Which would be correct in the sense that observers can't change the simstate, and should just view what the current sim values are. However I also considered #2459, if the values change otherwise than playerdoing, we will be in trouble. I presume fixing this there would need some event-based gui handling. We could argue this out of scope.

Some more history of the bug: The same issue was present when implementing the trade dialog in rP14417, however observers couldn't change perspective at that time. Then rP17675 fixed the issue partially, since it would only update the values on dialog open, and not while the dialog was open. rP23072 reverted to the original situation after rP14417, but kept the functionality that observers can change perspective.

bb updated this revision to Diff 13533.Sep 24 2020, 6:46 PM

Proper fixing indeed out of scope especially since more functions/buttons are probably affected.

binaries/data/mods/public/gui/session/trade/TradeButtonManager.js
11 ↗(On Diff #13533)

+. while at it.
Maybe a note about about when this fails? (E.g. when scripts change the value or players playing co-op.)

12 ↗(On Diff #13533)

The argument is still unused.

bb marked 2 inline comments as done.Sep 25 2020, 3:06 PM
bb added inline comments.
binaries/data/mods/public/gui/session/trade/TradeButtonManager.js
11 ↗(On Diff #13533)

adding of this player. The case where it fails must be the negation of the assumption, this should be obvious.

bb updated this revision to Diff 13539.Sep 25 2020, 3:06 PM
bb marked an inline comment as done.

comments

Freagarach accepted this revision.Sep 25 2020, 3:15 PM

Observers can now see the "proper" trading ratios, albeit they lag a bit behind in MP.

This revision is now accepted and ready to land.Sep 25 2020, 3:15 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Sep 25 2020, 11:35 PM