-
Notifications
You must be signed in to change notification settings - Fork 79
[GEN][ZH] Remove RTS_INTERNAL #1231
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
781e7e1
to
d7852ae
Compare
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/Win32Device/Common/Win32BIGFileSystem.cpp
Outdated
Show resolved
Hide resolved
idk if it would be better as a separate pr or as part of this one but there's a lot of files with commented out pragma optimize code that used the RTS_INTERNAL macro that can probably also be removed |
Yes that is removed with this change. |
Awesome! |
Because of the size, it will take some time to review this |
Much respect if you look at all lines. I trusted the script for the most part :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I'm looking at this correctly this is how the commits break down (not in order)
1] The first commit is exclusively for removes only the already commented out pragma optimize code so we can just mark that off as good
2] The second commit deals exclusively with instances where the condition is something form ±RTS_DEBUG and/or ±RTS_INTERNAL so we just need to double check that all the variations and their replacements were correct which they probably are
5] The fifth is just adding the python script that was used and doesn't do any functional changes itself but is relevant for review just to check/understand the other commits so unless there's something wrong with the other commits this one can be assumed good by default
4] The fourth is just removing rts_interal from cmake presets and .cmake files which is pretty straightforward and should be good but I will double check just in case
6,7] The sixth and seventh commits are just updates to the third
3] This leaves mainly just the third commit left that needs to be reviewed carefully
I'm guessing based on this PR, that I should go ahead and remove any mention of the internal build profile on the wiki? |
Yea after this is merged, RTS_INTERNAL is gone, because it essentially was duplicate of RTS_DEBUG. |
I have a PR for the Wiki lined up for when this PR gets merged: TheSuperHackers/GeneralsWiki#55 |
Yes, I'm reviewing all lines. |
GeneralsMD/Code/GameEngineDevice/Source/StdDevice/Common/StdBIGFileSystem.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/Win32Device/Common/Win32BIGFileSystem.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp
Outdated
Show resolved
Hide resolved
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngine/Source/GameLogic/Object/PartitionManager.cpp
Outdated
Show resolved
Hide resolved
Addressed review comments. |
All new review comments addressed. |
would you mind rebasing the fix up commits so that its easier to review the net changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of places still swapping RTS_INTERNAL macros with RTS_PROFILE or RTS_DEBUG instead of removing or disabling the blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we enabling vtune for debug builds? In fact this vtune code is obsolete and should be removed in a future pr. vtuneapi.dll doesn't even ship with vtune anymore. This outdated api is going to cause issues on modern windows
Why are we enabling VTune for Debug builds?
VTune integration relies on vtuneapi.dll, which is now obsolete and no longer ships with Intel VTune Profiler. The API is unsupported and may cause compatibility issues on modern Windows. I recommend removing the vtune code in a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are looking at an older commit. It was changed to RTS_PROFILE after SkyAero commented on it.
The Vtune stuff can be removed in a follow up change if that DLL is obsolete or unobtainable.
GeneralsMD/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this macro doesn't do anything without debug logging enabled which would not be desirable in profile build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to enable and use logging DEBUG_LOGGING with RTS_PROFILE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah but it doesn't make sense to do that because the logging slows down performance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I used logging for profiling in Python before. It works :D
@@ -1409,7 +1409,7 @@ VertexFormatXYZUV1 *StreakRendererClass::getVertexBuffer(unsigned int number) | |||
m_vertexBufferSize = numberToAlloc; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should look into why this wasn't enabled in debug builds originally before adding it to them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it writes deadbeef bytes which clearly is meant to be for revieweing memory in debugger.
@@ -677,7 +677,7 @@ void ConnectionManager::processChat(NetChatCommandMsg *msg) | |||
|
|||
void ConnectionManager::processFile(NetFileCommandMsg *msg) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd decision here that breaks convention and should probably be in a different pr. For now just replace it with #if 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EA wrote it like that so that the UnicodeString is not built in Release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we can remove the vtune stuff later
935559c
to
67607c4
Compare
Rebased and commits squashed. MERGE WITH REBASE |
67607c4
to
11f108e
Compare
Merge with Rebase
This change removes RTS_INTERNAL. It is mostly done by script. The commits are split into reviewable chunks.