Page MenuHomeWildfire Games

Cleanup MikktspaceWrap
ClosedPublic

Authored by Stan on May 9 2019, 4:42 PM.

Details

Reviewers
vladislavbelov
Silier
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP22316: Cleanup MikktspaceWrapper.
Summary
  • Cleanup Included.
  • Use forward declaration.
  • Remove undefined behavior refs #5288.
  • Use static_cast<> instead of c casts.
  • null → Nullptr.
  • Use Pascal case for functions.
  • Make variables const.
  • Remove duplication.
Test Plan

Test with and without precompiled headers.

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

Stan created this revision.May 9 2019, 4:42 PM
Vulcan added a comment.May 9 2019, 4:43 PM

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

Linter detected issues:
Executing section Source...

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

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

Stan updated this revision to Diff 7945.May 9 2019, 5:14 PM
  • Remove extra prefix
  • Fix year
Vulcan added a comment.May 9 2019, 5:17 PM

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

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

Stan updated this revision to Diff 7946.May 9 2019, 5:18 PM
Stan edited the summary of this revision. (Show Details)
  • Remove trailing whitespaces
Vulcan added a comment.May 9 2019, 5:20 PM

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

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

Silier added inline comments.May 9 2019, 9:04 PM
source/renderer/MikktspaceWrap.cpp
65 ↗(On Diff #7946)

I am only thinking, should be here (and on another places as well) check if we are not out of range?
Or we will (in theory we can) rely on that it will be always called with valid indexes ?

Stan added inline comments.May 10 2019, 8:37 AM
source/renderer/MikktspaceWrap.cpp
65 ↗(On Diff #7946)

Well I guess it iterates on every face the model provides, so I don't see how it can be out of range.

Builds with and without precompiled headers on windows.
Models and textures in game looks fine (did not expect do break anything)

source/renderer/MikktspaceWrap.cpp
65 ↗(On Diff #7946)

yes, you are right :) (mikkspace.cpp keeps them in range so not needed)

source/renderer/MikktspaceWrap.h
53 ↗(On Diff #7946)

maybe should be changed to javadoc style

source/renderer/ModelRenderer.cpp
142 ↗(On Diff #7946)

while this file has changes already, can we make ++j ( to make it consistent with rest of the file :) )?
the same on line 120

Stan updated this revision to Diff 7955.May 10 2019, 5:52 PM
Stan marked 2 inline comments as done.
  • Add documentation
Stan marked 2 inline comments as done.May 10 2019, 5:52 PM
Stan added inline comments.
source/renderer/MikktspaceWrap.h
53 ↗(On Diff #7946)

Would be doxygen, but yes ;)

source/renderer/ModelRenderer.cpp
142 ↗(On Diff #7946)

Good call :)

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

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

Silier accepted this revision.May 12 2019, 6:21 PM
This revision is now accepted and ready to land.May 12 2019, 6:21 PM
This revision was automatically updated to reflect the committed changes.
Stan marked 2 inline comments as done.
Owners added a subscriber: Restricted Owners Package.May 29 2019, 12:06 AM