Skip to content
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

Modernize headers #1743

Merged
merged 6 commits into from
Oct 10, 2024
Merged

Modernize headers #1743

merged 6 commits into from
Oct 10, 2024

Conversation

Xeeynamo
Copy link
Owner

@Xeeynamo Xeeynamo commented Oct 5, 2024

Extract some commits out of #1701 and avoid the part that will lead to a build failure (fixed in #1738).

Now every header uses OVL_EXPORT.

@Xeeynamo Xeeynamo force-pushed the modernize-headers branch 3 times, most recently from 9db7d57 to 9bef86b Compare October 5, 2024 21:39
@sozud
Copy link
Collaborator

sozud commented Oct 6, 2024

Looks like zero sized array issues


D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(11,14): error C2133: 'spriteBanks': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(12,16): error C2133: 'cluts': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(13,18): error C2133: 'rooms_layers': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(14,17): error C2133: 'gfxBanks': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]

Copy link
Collaborator

@sozud sozud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good once it compiles

@Xeeynamo
Copy link
Owner Author

Xeeynamo commented Oct 6, 2024

Looks like zero sized array issues


D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(11,14): error C2133: 'spriteBanks': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(12,16): error C2133: 'cluts': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(13,18): error C2133: 'rooms_layers': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]
D:\a\sotn-decomp\sotn-decomp\src\st\wrp\header.c(14,17): error C2133: 'gfxBanks': unknown size [D:\a\sotn-decomp\sotn-decomp\pc\core.vcxproj]

I cannot come up with any fix for that unfortunately. I am hitting another wall.

@Xeeynamo
Copy link
Owner Author

Xeeynamo commented Oct 6, 2024

I cherry-picked the commit from #1754 and it is now green 🟢

@sozud
Copy link
Collaborator

sozud commented Oct 6, 2024

I cannot come up with any fix for that unfortunately. I am hitting another wall.

Can the tool not detect the size of the arrays and emit a #define SPRITE_BANKS_SIZE to avoid these?

@Xeeynamo
Copy link
Owner Author

Xeeynamo commented Oct 6, 2024

I cannot come up with any fix for that unfortunately. I am hitting another wall.

Can the tool not detect the size of the arrays and emit a #define SPRITE_BANKS_SIZE to avoid these?

For the sprite banks size alone it might work, even if it is not that scalable. We would need to do that too to spriteBanks, Cluts, rooms_layers. And we might have the same issue somewhere else. I feel we're playing a game catching up with these issues. MSDN recommends to add /c and remove /Za but that does not work. At the same time I do not want to stall or try circumnavigating certain issues that will prevent moddability just to please a compiler that we do not know when (and if) the flags are going to get fixed.

@sozud
Copy link
Collaborator

sozud commented Oct 10, 2024

It looks like some of the CI steps are being skipped due to the conflicts?

@Xeeynamo
Copy link
Owner Author

It looks like some of the CI steps are being skipped due to the conflicts?

solved

Copy link
Collaborator

@sozud sozud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for fixing the msvc issues

@Xeeynamo Xeeynamo merged commit a915ff0 into master Oct 10, 2024
16 checks passed
@Xeeynamo Xeeynamo deleted the modernize-headers branch October 10, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants