Paths

Table of Contentst

Differential D946

Properly handle map resizes in Atlas
ClosedPublic
Actions

Authored by Itms on Oct 2 2017, 9:58 PM.

Details

Reviewers

vladislavbelov

Commits

rP21675: Properly handle map resizes in Atlas, fixes #4800.

Trac Tickets

#4800

Summary

This patch adds a necessary reinitialization of passability grid and dirtiness grid for the pathfinder. This fixes an assertion failure in Atlas when resizing the map and starting the simulation, as well as an unreported segmentation fault that would happen a few lines after the assertion failure.

It also adds a check to a helper function in Grid.

Test Plan

Test that #4800 doesn't happen.
Test resizing of maps in Atlas some more.
Test funky things like terrain updates mid-game with the Extinct Volcano map.

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

Itms created this revision.Oct 2 2017, 9:58 PM

Build is green

Updating workspaces...
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimation]’:
FCollada/FCDocument/FCDLibrary.cpp:149:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
  const T* cptr = ((const FCDLibrary<T>*)l1)->GetEntity(0);
           ^
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDAnimationClip]’:
FCollada/FCDocument/FCDLibrary.cpp:150:34:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDCamera]’:
FCollada/FCDocument/FCDLibrary.cpp:151:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDController]’:
FCollada/FCDocument/FCDLibrary.cpp:152:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEffect]’:
FCollada/FCDocument/FCDLibrary.cpp:153:27:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDEmitter]’:
FCollada/FCDocument/FCDLibrary.cpp:154:28:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDForceField]’:
FCollada/FCDocument/FCDLibrary.cpp:155:31:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDGeometry]’:
FCollada/FCDocument/FCDLibrary.cpp:156:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDImage]’:
FCollada/FCDocument/FCDLibrary.cpp:157:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDLight]’:
FCollada/FCDocument/FCDLibrary.cpp:158:26:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:159:29:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDSceneNode]’:
FCollada/FCDocument/FCDLibrary.cpp:160:30:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsModel]’:
FCollada/FCDocument/FCDLibrary.cpp:161:33:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ set but not used [-Wunused-but-set-variable]
FCollada/FCDocument/FCDLibrary.cpp: In instantiation of ‘void LibraryExport() [with T = FCDPhysicsMaterial]’:
FCollada/FCDocument/FCDLibrary.cpp:162:36:   required from here
FCollada/FCDocument/FCDLibrary.cpp:141:11: warning: variable ‘cptr’ s

http://jenkins-master:8080/job/phabricator/2099/ for more details.

Executing section Default...
Executing section Source...
Executing section JS...
Executing section XML GUI...
Executing section Python...
Executing section Perl...

http://jenkins-master:8080/job/phabricator_lint/572/ for more details.

Harbormaster completed remote builds in B3325: Diff 3838.Oct 2 2017, 10:48 PM

vladislavbelov added a reviewer: Restricted Owners Package.Oct 22 2017, 6:08 PM

It seems like a code duplication, you do the same thing as here: CCmpPathfinder.cpp#L462. I think it'd be better to make a function, or refactoring the code to prevent repeating. Currently you have 2 places to create m_Grid during one direct callstack.

This revision now requires changes to proceed.Nov 4 2017, 10:44 AM

vladislavbelov added inline comments.Nov 4 2017, 10:45 AM

source/simulation2/helpers/Grid.h
156 ↗	(On Diff #3838)	Maybe in elexis's js style? bool compare_sizes(const Grid<U>* g) const { return g && m_W == g->m_W && m_H == g->m_H; }

In D946#39618, @vladislavbelov wrote:

It seems like a code duplication, you do the same thing as here: CCmpPathfinder.cpp#L462. I think it'd be better to make a function, or refactoring the code to prevent repeating. Currently you have 2 places to create m_Grid during one direct callstack.

Hm you raise a valid point but I don't think it's actual code duplication: creating the grid and the dirtiness one is not the same thing as creating the grid, the dirtiness one and the terrain. I would like to avoid having spaghetti code with helper functions in that file. However I agree the callstack is now very ugly and some refactoring would help, but it's a big work. I'll try to think about it but in the meantime I don't think this fix is too bad. Let me know what you think, we can also discuss it some day on IRC.

+1 for the improvement to the compare_sizes function!

I'd be happy to have one crash less. If the addition is duplication, it's at least only a handful of lines.

source/simulation2/helpers/Grid.h
156 ↗	(On Diff #3838)	I would agree, although I don't see anything specific to JS :-)

I'd like to have a new review on this. I improved the style as requested and I really think the new code is not an actual duplicate of existing code.

Build failure - The Moirai have given mortals hearts that can endure.

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

Harbormaster failed remote builds in B5030: Diff 5825!Feb 18 2018, 4:56 PM

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

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

Harbormaster completed remote builds in B5030: Diff 5825.Feb 18 2018, 6:01 PM

Since this only address the pathfinder, there is no chance this fixes #5059, right?

In D946#55264, @elexis wrote:

Since this only address the pathfinder, there is no chance this fixes #5059, right?

Hm no I don't think the issues are directly related. ?

In D946#53536, @Itms wrote:

I'd like to have a new review on this. I improved the style as requested and I really think the new code is not an actual duplicate of existing code.

(I'll try the spawn-cheatcode: @vladislavbelov )

vladislavbelov added inline comments.Mar 22 2018, 11:45 PM

source/simulation2/components/CCmpPathfinder.cpp
1 ↗	(On Diff #5825)	Bump the year.
source/simulation2/helpers/Grid.h
1 ↗	(On Diff #5825)	Bump the year.

Stan added a subscriber: Stan.Mar 22 2018, 11:48 PM

Stan added inline comments.

source/simulation2/components/CCmpPathfinder.cpp
560 ↗	(On Diff #5825)	Just curious, why do we need to instantiate it with the new keyword ? Is it because it's assigned to a class member ?

vladislavbelov accepted this revision as: vladislavbelov.Mar 22 2018, 11:53 PM

vladislavbelov added inline comments.

source/simulation2/components/CCmpPathfinder.cpp
560 ↗	(On Diff #5825)	Because we allocate an object with different size.

Itms removed reviewers: Restricted Owners Package, Restricted Owners Package.Apr 8 2018, 10:54 PM

Itms added inline comments.

source/simulation2/components/CCmpPathfinder.cpp
560 ↗	(On Diff #5825)	Additional answer in case this is not totally clear: `m_Grid` is a pointer so `new` is needed to allocate on the heap. Else if one created a `Grid tmp` and assigned `m_Grid = &tmp`, it would be destroyed when `tmp` goes out of scope. Vlad's answer is the answer to "why does `m_Grid` have to be a pointer".