Page MenuHomeWildfire Games

Move Ogre3D GLSL Preprocessor to a third_party folder and restore its original name. Also restore the Wrapper name to reflect what it's wrapping and move it to renderer
ClosedPublic

Authored by Stan on Sep 29 2019, 7:00 PM.

Details

Reviewers
elexis
Angen
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Commits
rP23215: Move Ogre3D GLSL Preprocessor to a third_party folder and restore its original…
Summary

Following the discussion on rP9121:
Currently we use (kinda outdated) files from the ogre engine to parse our GLSL code.
It would be nice to put them in a third party folder so they can be upgraded later.
This patch does step 1 "moving" and the next one will do the "update part"

Test Plan

Check that the game still builds and nothing is broken.
Agree the file needs to be moved.
Test that shader errors are still reported
Test that shaders still work

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

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Linter detected issues:
Executing section Source...

source/graphics/OgreGLSLPreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/OgreGLSLPreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.

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

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

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

Linter detected issues:
Executing section Source...

source/graphics/OgreGLSLPreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/OgreGLSLPreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.

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

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

Stan updated this revision to Diff 10111.Oct 8 2019, 1:47 PM

Rename Wrapper to fit the classes in that folder.

Vulcan added a comment.Oct 8 2019, 1:49 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/427/display/redirect

Vulcan added a comment.Oct 8 2019, 1:51 PM

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

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

Stan updated this revision to Diff 10113.Oct 8 2019, 1:56 PM

Include fix.

Vulcan added a comment.Oct 8 2019, 1:58 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/428/display/redirect

Stan updated this revision to Diff 10114.Oct 8 2019, 2:05 PM

Fix ifndef now that the file got back its name.

Vulcan added a comment.Oct 8 2019, 2:05 PM

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

Linter detected issues:
Executing section Source...

source/graphics/PreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.

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

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

Vulcan added a comment.Oct 8 2019, 2:07 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/429/display/redirect

Vulcan added a comment.Oct 8 2019, 2:14 PM

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

Linter detected issues:
Executing section Source...

source/graphics/PreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.

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

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

Stan added a reviewer: elexis.Tue, Nov 12, 7:42 PM
Stan added subscribers: linkmauve, Angen.EditedMon, Dec 2, 4:42 PM

@Angen thoughts on this patch? All it does is move code to third party.
@linkmauve in case you want to look at the GLSL preprocessor code
For the record I chatted on the Ogre Gitter, and they are still making some fixes to it + the ones we currently have because we are a lot versions behind...

@StanleySweet then you might be interested in this: https://github.com/OGRECave/ogre/issues/614
Angen requested changes to this revision.Wed, Dec 4, 10:02 AM
Angen added inline comments.
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
236 ↗(On Diff #10114)

LOGERROR is missing :)

This revision now requires changes to proceed.Wed, Dec 4, 10:02 AM
Angen added inline comments.Wed, Dec 4, 10:03 AM
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
234 ↗(On Diff #10114)

static cast iTokenLen?

Stan requested review of this revision.Wed, Dec 4, 10:06 AM
Stan marked 2 inline comments as done.
Stan added inline comments.
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
234 ↗(On Diff #10114)

Original code, that was restored.

236 ↗(On Diff #10114)

Original code, that was restored.

Angen accepted this revision.Wed, Dec 4, 9:45 PM

Tests are passing.
Shader errors are reported.
From what I have seen shaders do work as without patch.
If it is third party file it should be in that folder.

This revision is now accepted and ready to land.Wed, Dec 4, 9:45 PM
Angen added inline comments.Wed, Dec 4, 9:47 PM
source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h
1 ↗(On Diff #10114)

here

elexis added a comment.Thu, Dec 5, 2:29 AM
In D2338#102497, @Angen wrote:

Tests are passing.
Shader errors are reported.
From what I have seen shaders do work as without patch.

I can't tell from that if you have read the patch too or only blackbox tested it.

If it is third party file it should be in that folder.

Does accepting mean trusting the author or verifying that the patch is correct?
There have been different models proposed for the review system.
In one model the patch is verified (i.e. somehow attempted to be proven to be correct and complete), in another model it's only black box testing (apply the patch and then accept if there are no errors while running the game briefly).
If the latter is the chosen model, then one cannot identify whether the idea of the patch is correct or whether the patch should be changed it works only at face value but has a coneptual issues.
If the objective of the review system is to improve code quality over just one author testing that his patch works at face value, nothing is gained while more people pay with more time.

Noting how the correctness of the implementation has been examined when accepting a patch allows:

  • determining which aspects of a patch had been examined
  • locating possible oversights
  • eases bugtracking and revisiting of the code later on (even for the author and reviewers later on as well as one loses memory of minor code decisions after a while).

For example for this patch:

  • Are the premake and logger change correct and complete?
  • Are the changes to OgreGLSLPreprocessor.cpp complete to achieve the purpose of the diff to this file?
  • I couldn't follow the many iterations. Is the latest name choice the best of the name choices iterated?
Angen added a comment.Thu, Dec 5, 7:20 PM

Reason of this diff is to move Preprocessor file to third party files, what is completed and moving a file does not break its functionality.

This patch does step 1 "moving" and the next one will do the "update part"

File was included in rP9121 using header file OgreGLSLPreprocessor.h . In fact, file itself has name OgreGLSLPreprocessor.cpp in https://github.com/OGRECave/ogre so naming is adequate.

I suppose that removing dependency from pyrogenesis was done based on

I suppose third party libs should not depend on Pyrogenesis code.

what makes sense, so functionality from logger was almost restored because there is missing line as I pointed out on IRC

09:10 < Angen> https://code.wildfiregames.com/rP9121
09:10 < Angen> you are still missing this line
09:10 < Angen> LogManager::getSingleton ().logMessage (line);

But since that would require to add another file OgreLogManager.h and default logger is replaced in PreprocessorWrapper.cpp with custom one

CPreprocessor::ErrorHandler = CPreprocessorWrapper::PyrogenesisShaderError;

it would be wasted include.

Custom logger is basically copy-paste from one replaced with previous code, so again moving same logic somewhere else what means functionality is not changed as long as correct version of logger is assigned what is done.

There have been some other tweaks to OgreGLSLPreprocessor.cpp like removing namespace, using debug_assert or style changes.

Also there have been replaced debug_assert -> ENSURE.

But main objective is to move that file to third party folder and update it in next step.


One thing I now noticed is that "third_party/ogre3d_preprocessor" should be preferably included in "graphics" project rather than in "atlas" as it should be included somewhere to get cpp part build.

Stan updated this revision to Diff 10473.Thu, Dec 5, 7:55 PM
Stan marked 2 inline comments as done.

Switch includes, fix premake.

Vulcan added a comment.Thu, Dec 5, 7:55 PM

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

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

Vulcan added a comment.Thu, Dec 5, 7:56 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/680/display/redirect

Stan updated this revision to Diff 10474.Thu, Dec 5, 8:29 PM

Add missing file.

Vulcan added a comment.Thu, Dec 5, 8:30 PM

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

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

Vulcan added a comment.Thu, Dec 5, 8:30 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/681/display/redirect

Stan updated this revision to Diff 10476.Thu, Dec 5, 8:55 PM

Try with ps/trunk/ prefix

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Thu, Dec 5, 8:55 PM
Vulcan added a comment.Thu, Dec 5, 8:55 PM

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

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

Vulcan added a comment.Thu, Dec 5, 8:56 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/682/display/redirect

Stan updated this revision to Diff 10478.Thu, Dec 5, 10:57 PM
Stan edited the summary of this revision. (Show Details)

First attempt at using arc

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

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

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/683/display/redirect

Stan updated this revision to Diff 10490.Fri, Dec 6, 4:21 PM

Test with patched arc

Vulcan added a comment.Fri, Dec 6, 4:21 PM

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

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

Vulcan added a comment.Fri, Dec 6, 4:22 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/692/display/redirect

Stan updated this revision to Diff 10492.Fri, Dec 6, 4:41 PM

try to pass --ignore-eol-style to diff.exe in git-bash.exe

Vulcan added a comment.Fri, Dec 6, 4:42 PM

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

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

Vulcan added a comment.Fri, Dec 6, 4:42 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/693/display/redirect

Stan updated this revision to Diff 10493.Fri, Dec 6, 4:45 PM

Try with gnu diff and --strip-trailing-cr

Vulcan added a comment.Fri, Dec 6, 4:45 PM

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

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

Vulcan added a comment.Fri, Dec 6, 4:46 PM

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/694/display/redirect

Angen requested changes to this revision.EditedFri, Dec 6, 8:36 PM
Angen added a subscriber: Freagarach.

linux is not able to apply this anymore
confirmed by @Freagarach

This revision now requires changes to proceed.Fri, Dec 6, 8:36 PM
Stan updated this revision to Diff 10503.Sat, Dec 7, 11:38 AM

Try with git this time...

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

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

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/699/display/redirect

Stan updated this revision to Diff 10504.Sat, Dec 7, 11:44 AM

Fix include

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/700/display/redirect

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

Linter detected issues:
Executing section Source...

source/graphics/PreprocessorWrapper.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2015" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

Stan updated this revision to Diff 10505.Sat, Dec 7, 12:00 PM

Rebase...

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

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/701/display/redirect

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

Linter detected issues:
Executing section Source...

source/graphics/PreprocessorWrapper.cpp
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2015" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|   1| /*·Copyright·(C)·2019·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2013" year instead of "2019"

source/graphics/PreprocessorWrapper.h
|  29| class·CPreprocessorWrapper
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCPreprocessorWrapper{' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

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

Angen accepted this revision.Sat, Dec 7, 12:15 PM
This revision is now accepted and ready to land.Sat, Dec 7, 12:15 PM