Page MenuHomeWildfire Games
Differential D2338

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
Actions

Authored by Stan on Sep 29 2019, 7:00 PM.
Tags
None
Referenced Files
F5229120: D2338.id10028.diff
Thu, Sep 26, 9:46 PM
Unknown Object (File)
Wed, Sep 25, 8:55 AM
Unknown Object (File)
Sat, Sep 21, 12:34 AM
Unknown Object (File)
Fri, Sep 20, 4:30 AM
Unknown Object (File)
Wed, Sep 18, 10:31 PM
Unknown Object (File)
Wed, Sep 18, 9:36 AM
Unknown Object (File)
Tue, Sep 17, 3:00 PM
Unknown Object (File)
Mon, Sep 16, 10:37 PM
View All 43 Files
Subscribers
elexis
Freagarach
linkmauve
Silier
vladislavbelov
Restricted Owners Package
Restricted Owners Package

Details

Reviewers
elexis
Silier
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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Vulcan added a comment.Oct 8 2019, 12:31 PM

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

Harbormaster completed remote builds in B9767: Diff 10107.Oct 8 2019, 12:31 PM
Vulcan added a comment.Oct 8 2019, 12:39 PM

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

Harbormaster completed remote builds in B9768: Diff 10108.Oct 8 2019, 12:39 PM
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

Harbormaster failed remote builds in B9769: Diff 10111!Oct 8 2019, 1:49 PM
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

Harbormaster completed remote builds in B9770: Diff 10113.Oct 8 2019, 2:05 PM
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

Harbormaster completed remote builds in B9771: Diff 10114.Oct 8 2019, 2:14 PM
Stan added a reviewer: elexis.Nov 12 2019, 7:42 PM
Stan added subscribers: linkmauve, Silier.EditedDec 2 2019, 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
Silier requested changes to this revision.Dec 4 2019, 10:02 AM
Silier added inline comments.
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
236

LOGERROR is missing :)

This revision now requires changes to proceed.Dec 4 2019, 10:02 AM
Silier added inline comments.Dec 4 2019, 10:03 AM
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
234

static cast iTokenLen?

Stan requested review of this revision.Dec 4 2019, 10:06 AM
Stan marked 2 inline comments as done.
Stan added inline comments.
source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp
234

Original code, that was restored.

236

Original code, that was restored.

Silier accepted this revision.Dec 4 2019, 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.Dec 4 2019, 9:45 PM
Silier added inline comments.Dec 4 2019, 9:47 PM
source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h
1

here

elexis added a comment.Dec 5 2019, 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?
Silier added a comment.Dec 5 2019, 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.Dec 5 2019, 7:55 PM
Stan marked 2 inline comments as done.

Switch includes, fix premake.

Vulcan added a comment.Dec 5 2019, 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

Harbormaster failed remote builds in B10186: Diff 10473!Dec 5 2019, 7:55 PM
Vulcan added a comment.Dec 5 2019, 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.Dec 5 2019, 8:29 PM

Add missing file.

Vulcan added a comment.Dec 5 2019, 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

Harbormaster failed remote builds in B10187: Diff 10474!Dec 5 2019, 8:30 PM
Vulcan added a comment.Dec 5 2019, 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.Dec 5 2019, 8:55 PM

Try with ps/trunk/ prefix

Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Dec 5 2019, 8:55 PM
Vulcan added a comment.Dec 5 2019, 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

Harbormaster failed remote builds in B10188: Diff 10476!Dec 5 2019, 8:55 PM
Vulcan added a comment.Dec 5 2019, 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.Dec 5 2019, 10:57 PM
Stan edited the summary of this revision. (Show Details)

First attempt at using arc

Vulcan added a comment.Dec 5 2019, 10:58 PM

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

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

Harbormaster failed remote builds in B10190: Diff 10478!Dec 5 2019, 10:58 PM
Vulcan added a comment.Dec 5 2019, 11:06 PM

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.Dec 6 2019, 4:21 PM

Test with patched arc

Vulcan added a comment.Dec 6 2019, 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

Harbormaster failed remote builds in B10200: Diff 10490!Dec 6 2019, 4:21 PM
Vulcan added a comment.Dec 6 2019, 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.Dec 6 2019, 4:41 PM

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

Vulcan added a comment.Dec 6 2019, 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

Harbormaster failed remote builds in B10202: Diff 10492!Dec 6 2019, 4:42 PM
Vulcan added a comment.Dec 6 2019, 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.Dec 6 2019, 4:45 PM

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

Vulcan added a comment.Dec 6 2019, 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

Harbormaster failed remote builds in B10203: Diff 10493!Dec 6 2019, 4:45 PM
Vulcan added a comment.Dec 6 2019, 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

Silier requested changes to this revision.EditedDec 6 2019, 8:36 PM
Silier added a subscriber: Freagarach.

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

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

Try with git this time...

Vulcan added a comment.Dec 7 2019, 11:39 AM

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

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

Harbormaster failed remote builds in B10215: Diff 10503!Dec 7 2019, 11:39 AM
Vulcan added a comment.Dec 7 2019, 11:39 AM

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.Dec 7 2019, 11:44 AM

Fix include

Vulcan added a comment.Dec 7 2019, 11:46 AM

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

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

Vulcan added a comment.Dec 7 2019, 11:51 AM

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

Harbormaster completed remote builds in B10216: Diff 10504.Dec 7 2019, 11:51 AM
Stan updated this revision to Diff 10505.Dec 7 2019, 12:00 PM

Rebase...

Vulcan added a comment.Dec 7 2019, 12:02 PM

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

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

Vulcan added a comment.Dec 7 2019, 12:07 PM

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

Harbormaster completed remote builds in B10217: Diff 10505.Dec 7 2019, 12:07 PM
Silier accepted this revision.Dec 7 2019, 12:15 PM
This revision is now accepted and ready to land.Dec 7 2019, 12:15 PM
Closed by commit rP23215: Move Ogre3D GLSL Preprocessor to a third_party folder and restore its original… (authored by Stan). · Explain WhyDec 7 2019, 12:48 PM
This revision was automatically updated to reflect the committed changes.
Stan added a commit: rP23215: Move Ogre3D GLSL Preprocessor to a third_party folder and restore its original….

Revision Contents
Changeset List

PathSize
build/
premake/
3 lines
source/
graphics/
2 lines
2 lines
2 lines
graphics/
ps/
8 lines
15 lines
ps/
tests/
third_party/
ogre3d_preprocessor/
ps/
541 lines
OgreGLSLPreprocessor.cpp
Preprocessor.cpp
9 lines
tests/
10 lines

Diff 10503

View Options

build/premake/premake5.lua

Loading...
View Options

source/graphics/MaterialManager.cpp

Loading...
View Options

source/graphics/PreprocessorWrapper.h

Loading...
View Options

source/graphics/PreprocessorWrapper.cpp

Loading...
View Options

source/graphics/ShaderManager.cpp

Loading...
View Options

source/graphics/ShaderProgram.cpp

Loading...
View Options

source/ps/Preprocessor.h

Loading...
View Options

source/ps/Preprocessor.cpp

Loading...
View Options

source/ps/PreprocessorWrapper.h

Loading...
View Options

source/ps/PreprocessorWrapper.cpp

Loading...
View Options

source/ps/tests/test_Preprocessor.h

Loading...
View Options

source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.h

Loading...
View Options

source/third_party/ogre3d_preprocessor/OgreGLSLPreprocessor.cpp

Loading...
View Options

source/third_party/ogre3d_preprocessor/tests/test_Preprocessor.h

Loading...
Wildfire Games · Phabricator