Handle MPQ maps without HM3W headers - #1258
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73c59e40de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // so use a new header in that case; writeToMapFile will insert it before | ||
| // the archive. | ||
| MapHeader mapHeader = startsWithMpqArchive(targetMap) | ||
| ? new MapHeader() |
There was a problem hiding this comment.
When a headerless map configures only buildMapData.name() and relies on the existing war3map.w3i player definitions, this constructs a header whose maximum-player count remains the MapHeader default of zero (the new test explicitly confirms that value). The inserted HM3W header consequently advertises a zero-player map instead of preserving its existing capacity; populate the synthesized header from the already-extracted W3I metadata before writing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in d54d514. Header synthesis now receives the existing W3I player count and preserves it when only map name configuration changes; the regression test asserts maxPlayersCount=3.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d54d514371
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the archive. | ||
| boolean hasNoMapHeader = startsWithMpqArchive(targetMap); | ||
| MapHeader mapHeader = hasNoMapHeader | ||
| ? new MapHeader() |
There was a problem hiding this comment.
When a headerless map configures buildMapData.players() but leaves buildMapData.name() blank, this creates a MapHeader with its default map-name field and writes it in front of the archive. The resulting HM3W header therefore no longer advertises the existing name stored in war3map.w3i; pass the W3I map name into this method and populate it when synthesizing the header, analogous to the preserved player count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 659e909. Header synthesis now receives the existing W3I map name and preserves it when player configuration changes without an explicit map name; the regression test covers that case.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659e9091ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| MapHeader mapHeader = hasNoMapHeader | ||
| ? new MapHeader() | ||
| : MapHeader.ofFile(targetMap); |
There was a problem hiding this comment.
For a headerless MPQ whose W3I has nonzero map flags, such as fixed/custom forces or minimap settings, new MapHeader() leaves the header flags at their default while only the map name and player count are restored before writing. The synthesized HM3W metadata therefore disagrees with war3map.w3i, so clients that inspect the header can treat those flags as unset; pass the existing W3I flags into this path and copy them into the new header.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7c10423. Header synthesis now copies the existing W3I flags via getFlags().toInt(), with regression coverage asserting the flags survive insertion.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex review loop complete: 4 review rounds on successive heads. Fixed the reported P2s by preserving W3I player count, map name, and flags when synthesizing an HM3W header for a headerless MPQ. Focused WurstBuildConfigTests pass, GitHub CI is green on Ubuntu and Windows, and the latest Codex review of 7c10423 found no major issues. Ready to merge. |
1 participant
Summary
HM3Wprefix.MapHeader.writeToMapFilethen inserts it before the archive.Checks
.\gradlew.bat test --tests tests.wurstscript.tests.WurstBuildConfigTests --no-daemon— passed.git diff --check— passed.Known gaps