Page MenuHomeWildfire Games

Cleanups BoundingBoxAxisAligned before refactoring

Authored by vladislavbelov on Jun 3 2019, 9:58 PM.


Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
rP22372: Cleanups BoundingBoxAxisAligned and fixes coding styles a bit.


Test Plan
  1. Apply the patch and the game with tests
  2. Run tests
  3. Make sure that all related tests are passed
  4. Run the game
  5. Make sure that everything is rendered as before

Diff Detail

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

Event Timeline

vladislavbelov created this revision.Jun 3 2019, 9:58 PM

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

Linter detected issues:
Executing section Source...

| 213| »   void·UpdatePropListRecursive(CModelAbstract*·model);
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Invalid number of character '{' when these macros are defined: 'MESSAGESSETUP_NOTFIRST'.
Executing section JS...
Executing section cli...

Link to build:

wraitii added a subscriber: wraitii.Jun 5 2019, 7:09 PM

This looks good.

38 ↗(On Diff #8297)

Needless comment imo? When we've had inline hints like that we generally wrapped them such /* */anyways.

36 ↗(On Diff #8297)

I think those should be private and matrix should probably be our friend - or alternatively just define these inline in BoundingBoxAligned.cpp if that's the only place where it's used.
I don't see what sane 'client' code would need this offhand.

vladislavbelov added inline comments.Jun 5 2019, 8:36 PM
38 ↗(On Diff #8297)

I don't mind, it's more my habit.

36 ↗(On Diff #8297)

I don't see any reason to hide it from others. It's the same as FLT_MAX. But yes, it could be inlined.

It can be used in any kind of search, where we have undefined value before start.

wraitii accepted this revision.Jun 5 2019, 9:52 PM

Seems fine, passes tests, gets in-game with no changes that I can notice.

36 ↗(On Diff #8297)

Fair enough.

This revision is now accepted and ready to land.Jun 5 2019, 9:52 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Jun 12 2019, 10:23 PM