Page MenuHomeWildfire Games

Fix GetTerritoryPercentage when changing number of players
ClosedPublic

Authored by Silier on Jul 5 2019, 9:02 PM.

Details

Reviewers
Stan
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23510: Fix GetTerritoryPercentage when changing number of players
Summary

Error:
We check if player > m_TerritoryCellCounts.size(), but m_TerritoryCellCounts.size() == number of players and we use player as index
Plus we are recomputing territories after we check bounds what if we remove player can lead to out of bounds exception

Run Atlas in debug mode and lower number of players to 1 using arrow key or mouse wheel

Test Plan

Run Atlas in debug mode and lower number of players to 1 using arrow key or mouse wheel

Event Timeline

Silier created this revision.Jul 5 2019, 9:02 PM
Owners added a subscriber: Restricted Owners Package.Jul 5 2019, 9:02 PM
Silier added inline comments.Jul 5 2019, 9:05 PM
source/simulation2/components/CCmpTerritoryManager.cpp
584

we probably should check against number of players in game and if it is not out of scope then let to recompute territories

Vulcan added a comment.Jul 5 2019, 9:06 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpTerritoryManager.cpp
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2019" year instead of "2018"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/differential/1911/display/redirect

wraitii added a reviewer: Restricted Owners Package.Jul 11 2019, 3:04 PM
Stan requested changes to this revision.Dec 21 2019, 1:59 PM
Stan added a subscriber: Stan.

Maybe add a test?

source/simulation2/components/CCmpTerritoryManager.cpp
584

There is a flag for whether the territories are dirty. Maybe we should use that?

591

static_cast

This revision now requires changes to proceed.Dec 21 2019, 1:59 PM
Silier updated this revision to Diff 11284.Feb 5 2020, 7:43 PM

static cast

Silier added inline comments.Feb 5 2020, 7:44 PM
source/simulation2/components/CCmpTerritoryManager.cpp
584

well actually no, CalculateTerritories has early return if m_Territories exists, so it recomputes only if really needed.

Vulcan added a comment.Feb 5 2020, 7:49 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpTerritoryManager.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2019"
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1718/display/redirect

Silier updated this revision to Diff 11293.Feb 7 2020, 8:02 PM

comment, year, merge conditions

Vulcan added a comment.Feb 7 2020, 8:16 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1724/display/redirect

Stan added a comment.Feb 20 2020, 9:45 PM

Patch looks good other than that last comment. Not sure how often that error might occur, but I assume it's better to have correct code.

source/simulation2/components/CCmpTerritoryManager.cpp
589–590

// Territories may have been recalculated, check whether player is still there. ?

Silier updated this revision to Diff 11407.Feb 20 2020, 9:58 PM

comment

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1799/display/redirect

Stan accepted this revision.Feb 20 2020, 11:09 PM
This revision is now accepted and ready to land.Feb 20 2020, 11:09 PM
This revision was automatically updated to reflect the committed changes.