Page MenuHomeWildfire Games

Cleanup MikktspaceWrap
AcceptedPublic

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

Details

Reviewers
vladislavbelov
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
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
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 7420
Build 12074: Vulcan BuildJenkins

Event Timeline

Stan created this revision.Thu, May 9, 4:42 PM
Vulcan added a comment.Thu, May 9, 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.Thu, May 9, 5:14 PM
  • Remove extra prefix
  • Fix year
Vulcan added a comment.Thu, May 9, 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.Thu, May 9, 5:18 PM
Stan edited the summary of this revision. (Show Details)
  • Remove trailing whitespaces
Vulcan added a comment.Thu, May 9, 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

Angen added inline comments.Thu, May 9, 9:04 PM
source/renderer/MikktspaceWrap.cpp
65

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.Fri, May 10, 8:37 AM
source/renderer/MikktspaceWrap.cpp
65

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

Angen added a comment.Fri, May 10, 5:33 PM

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

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

source/renderer/MikktspaceWrap.h
46–47

maybe should be changed to javadoc style

source/renderer/ModelRenderer.cpp
142

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.Fri, May 10, 5:52 PM
Stan marked 2 inline comments as done.
  • Add documentation
Stan marked 2 inline comments as done.Fri, May 10, 5:52 PM
Stan added inline comments.
source/renderer/MikktspaceWrap.h
46–47

Would be doxygen, but yes ;)

source/renderer/ModelRenderer.cpp
142

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

Angen accepted this revision.Sun, May 12, 6:21 PM
This revision is now accepted and ready to land.Sun, May 12, 6:21 PM