Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 7, 2025
Merged

[GEN][ZH] Remove RTS_INTERNAL #1231

merged 5 commits into from
Jul 7, 2025
+1,086 −4,264

Conversation

xezon
Copy link

@xezon xezon commented Jul 5, 2025

Merge with Rebase

This change removes RTS_INTERNAL. It is mostly done by script. The commits are split into reviewable chunks.

@xezon xezon added Major Severity: Minor < Major < Critical < Blocker Build Anything related to building, compiling Refactor Edits the code with insignificant behavior changes, is never user facing Debug Is mostly debug functionality labels Jul 5, 2025
@xezon xezon force-pushed the xezon/remove-rts-internal-2 branch from 781e7e1 to d7852ae Compare July 5, 2025 12:37
@aliendroid1
Copy link

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

@xezon
Copy link
Author

xezon commented Jul 5, 2025

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.

@aliendroid1
Copy link

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!

@roossienb roossienb self-requested a review July 5, 2025 17:01
@roossienb
Copy link

Because of the size, it will take some time to review this

@xezon
Copy link
Author

xezon commented Jul 5, 2025

Much respect if you look at all lines. I trusted the script for the most part :)

aliendroid1
aliendroid1 reviewed Jul 5, 2025
View reviewed changes
Copy link

@aliendroid1 aliendroid1 left a comment •
edited
Loading

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

@Jaredl-Code
Copy link

I'm guessing based on this PR, that I should go ahead and remove any mention of the internal build profile on the wiki?

@xezon
Copy link
Author

xezon commented Jul 5, 2025

Yea after this is merged, RTS_INTERNAL is gone, because it essentially was duplicate of RTS_DEBUG.

@Jaredl-Code Jaredl-Code mentioned this pull request Jul 5, 2025
Open
@Jaredl-Code
Copy link

I have a PR for the Wiki lined up for when this PR gets merged: TheSuperHackers/GeneralsWiki#55

@roossienb
Copy link

Much respect if you look at all lines. I trusted the script for the most part :)

Yes, I'm reviewing all lines.

roossienb
roossienb requested changes Jul 6, 2025
View reviewed changes
@xezon
Copy link
Author

xezon commented Jul 6, 2025

Addressed review comments.

@xezon xezon requested a review from roossienb July 6, 2025 13:12
@xezon
Copy link
Author

xezon commented Jul 6, 2025

All new review comments addressed.

@aliendroid1
Copy link

aliendroid1 commented Jul 7, 2025 •
edited
Loading

would you mind rebasing the fix up commits so that its easier to review the net changes

aliendroid1
aliendroid1 suggested changes Jul 7, 2025
View reviewed changes
Copy link

@aliendroid1 aliendroid1 left a comment •
edited
Loading

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

Generals/Code/GameEngine/Source/GameLogic/ScriptEngine/ScriptEngine.cpp Outdated
Copy link

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.

Copy link
Author

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.

Generals/Code/GameEngine/Include/GameLogic/ScriptEngine.h Outdated
Copy link

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

Copy link
Author

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.

Copy link

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

Copy link
Author

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

Core/Libraries/Source/WWVegas/WW3D2/streakRender.cpp
@@ -1409,7 +1409,7 @@ VertexFormatXYZUV1 *StreakRendererClass::getVertexBuffer(unsigned int number)
m_vertexBufferSize = numberToAlloc;
}

Copy link

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

Copy link
Author

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.

Generals/Code/GameEngine/Source/GameNetwork/ConnectionManager.cpp
@@ -677,7 +677,7 @@ void ConnectionManager::processChat(NetChatCommandMsg *msg)

void ConnectionManager::processFile(NetFileCommandMsg *msg)
{
Copy link

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

Copy link
Author

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.

@xezon xezon requested a review from aliendroid1 July 7, 2025 07:41
aliendroid1
aliendroid1 approved these changes Jul 7, 2025
View reviewed changes
Copy link

@aliendroid1 aliendroid1 left a 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

@xezon xezon force-pushed the xezon/remove-rts-internal-2 branch from 935559c to 67607c4 Compare July 7, 2025 16:15
@xezon
Copy link
Author

xezon commented Jul 7, 2025

Rebased and commits squashed.

MERGE WITH REBASE

@xezon xezon force-pushed the xezon/remove-rts-internal-2 branch from 67607c4 to 11f108e Compare July 7, 2025 16:45
@xezon xezon merged commit 26bc2ca into TheSuperHackers:main Jul 7, 2025
14 checks passed
xezon added a commit that referenced this pull request Jul 7, 2025
@xezon
[GEN][ZH] Remove all RTS_INTERNAL epilogues (#1231)
79f8e9a
xezon added a commit that referenced this pull request Jul 7, 2025
@xezon
[GEN][ZH] Remove obsolete conditions for RTS_INTERNAL (#1231)
b342328
xezon added a commit that referenced this pull request Jul 7, 2025
@xezon
[GEN][ZH] Remove or repurpose RTS_INTERNAL specific code (#1231)
7909aee
xezon added a commit that referenced this pull request Jul 7, 2025
@xezon
[CMAKE] Remove RTS_BUILD_OPTION_INTERNAL (#1231)
4b6a0a1
@xezon xezon deleted the xezon/remove-rts-internal-2 branch July 7, 2025 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roossienb roossienb roossienb approved these changes

@aliendroid1 aliendroid1 aliendroid1 approved these changes

Assignees
No one assigned
Labels
Build Anything related to building, compiling Debug Is mostly debug functionality Major Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove RTS_INTERNAL
4 participants
@xezon @aliendroid1 @roossienb @Jaredl-Code