Exclude traveling units from combat and space calculations (#796) - #1223
Open
acato wants to merge 13 commits into
Open
Exclude traveling units from combat and space calculations (#796)#1223acato wants to merge 13 commits into
acato wants to merge 13 commits into
Conversation
Extends the trade route system to support Africa and Port Royal as off-map trade locations alongside Europe. Adds sentinel IDs, the isOffMapTradeLocation() helper, route validation, AI evaluation, Python UI support in the Trade Routes Advisor, and canSailToAfrica/ canSailToPortRoyal Python bindings. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Custom MinUnit-derived C++03 test framework with 35 tests across 6 suites covering EnumMap, JustInTimeArray, Coordinates, CvTradeRoute sentinels, and CvIdVector. Tests run at startup in Assert/Debug builds after XML load, logging results to Logs\WTPTests.log. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
C++ tests (10) validate XML cross-references at startup: profession yields, building classes, unit professions, father CivEffects/categories, yield costs, equipment amounts, terrain yields, and trade sentinels. Perl scripts: test_determinism.pl detects non-deterministic calls (rand/srand/time/GetTickCount) outside allowed files; test_text_keys.pl verifies TXT_KEY references in data XML have matching text definitions. Both run as part of the test_DllExport build target. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 3: MemoryStream (in-memory FDataStreamBase) with 23 round-trip tests covering primitives, strings, IDInfo, JustInTimeArray, and enum types. Fix XMLIntegrity test: allow NO_YIELD (-1) in profession yield lists, as the game code explicitly checks and skips it. Fix int-to-enum casts for VC++ 2003 strict type checking. Fix Savegame test: JustInTimeArray::Read() always allocates, even for empty arrays — removed incorrect !isAllocated() assertion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Set GAME_MODS_PATH in Makefile.settings to automatically xcopy the mod to the game's Mods directory as a post-build step. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Ships with automated trade routes to off-map destinations (Africa, Port Royal) would get stuck upon arrival or lose their route assignments when crossing the ocean. Four root causes fixed: - AI_update() now intercepts units in Port Royal (was only Europe/Africa) - AI_europeUpdate() handles AUTOMATE_TRANSPORT_ROUTES ships: sells cargo and crosses back automatically (previously only AUTOMATE_FULL was handled) - AI_transportMoveRoutes() routes to correct destination via new helper AI_sailToOffMapTradeDestination() instead of always sailing to Europe - setUnitTravelState() preserves automation and trade route assignments across group splits during ocean travel Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Phase 4: CvRandom determinism tests — seed reproducibility, range bounds, peek non-advancement, float output, and state round-trips (576 checks). Phase 5: Python-side test suite (WTPTests.py) validating the DLL-Python bridge and XML data consistency — profession yields, building classes, unit professions, father categories, terrain yields, civilization info. Runnable from debug console: import WTPTests; WTPTests.runAllTests() Also fixes WTPTests.log double-nesting (gDLL->logMsg already writes to the Logs/ directory). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
setCityYieldModifierString was missing the YIELD_LAW exclusion when applying the rebel yield modifier, causing a mismatch with getBaseYieldRateModifier which correctly excludes it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
EventTriggeredData::setRandomNumbers() creates a RandomContainer for each event in the trigger, generates a random number via getSorenRandNum, but never actually stores the container in m_RandomNumbers. The push_back call was missing, so m_RandomNumbers remained empty after the function completed. This caused getRandomNumber() and getRandomNumberForIndex() to always return 0 (the fallback default), which had two consequences: 1. Any Python PythonCanDo callback using getRandomNumberForIndex() for probability checks would always see 0. For example, canTriggerVolcanoDormant1() checks "getRandomNumberForIndex(0) < 250" which was always true (0 < 250), making the volcano dormant event fire 100% of the time instead of the intended ~25%. 2. The DLL-side TriggerChance fastpath in CvPlayer::canDoEvent() (line 14762) uses getRandomNumber(eEvent) to check event probability thresholds. With the vector always empty, this also always returned 0, bypassing intended probability gates for events validated through that code path. The fix adds the missing m_RandomNumbers.push_back(container) so that generated random numbers are actually persisted in the vector and available to both Python and C++ callers. Related: We-the-People-civ4col-mod#1205 (CvRandomInterfaceEvent tuple index out of range) The primary cause of We-the-People-civ4col-mod#1205 was an argsList index mismatch fixed in 38b7518, but this bug compounded the issue by making the random number check in canTriggerVolcanoDormant1 a no-op. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
In CvPlayer::initTriggeredData(), after a PythonCanDo callback returns successfully, the code reconstructs a Coordinates object from the trigger data (since Python may have modified it). However, line 14416 passed m_iPlotX as both the X and Y arguments: Coordinates coord(pTriggerData->m_iPlotX, pTriggerData->m_iPlotX); This meant pPlot was resolved to the wrong tile whenever the Python callback modified the trigger's plot coordinates and X != Y. The resolved plot would be at (X, X) instead of (X, Y), causing the event to target the wrong map location for any subsequent logic that uses pPlot (text generation, world news, event application). Events where PythonCanDo does not modify coordinates would be unaffected since pPlot was already correctly set before the callback. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Units traveling to/from Europe/Africa/Port Royal retain their plot coordinates but should not participate in gameplay on that tile. While getBestDefender already checked isOnMap(), the lower-level canDefend() did not — so getNumDefenders() and any direct callers of canDefend() would incorrectly count traveling units. Adding isOnMap() to canDefend() closes all gaps at once. Also fixed harbour and barracks space calculations in canMoveInto that iterated all plot units without checking travel state, causing traveling ships/troops to incorrectly consume space. Fixes We-the-People-civ4col-mod#796 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
devolution79
requested changes
Mar 30, 2026
| } | ||
|
|
||
| // Units traveling to/from Europe/Africa/Port Royal cannot defend | ||
| if (!isOnMap()) |
Member
There was a problem hiding this comment.
Thanks, this could very well be the fix for this issue. Please use the inline isOnMap_() variant.
| // we also not consider Units loaded on Ships | ||
| // we also not consider Units of other Nations | ||
| if (pLoopUnit != NULL && pLoopUnit->getDomainType() == DOMAIN_SEA) | ||
| if (pLoopUnit != NULL && pLoopUnit->getDomainType() == DOMAIN_SEA && pLoopUnit->isOnMap()) |
Member
There was a problem hiding this comment.
These additional checks do not belong here because they serves no purpose. A unit for which isOnMap() is false should never reach canMoveInto() unless there is already some serious bug elsewhere.
In fact, if such a check were needed at all, the correct place for it would be much higher up in the call chain. Units that are not on the map are excluded by the normal unit move action processing loop, except for the special case of units in port, where the port update function is called directly rather than going through CvUnit::AI_update().
Just revert the changes to this function.
- Use isOnMap_() inline variant in canDefend() per reviewer request - Remove isOnMap() checks from canMoveInto() as traveling units should never reach that function - the check belongs higher in the call chain Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Author
|
Thanks - Updated the PR with both changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Units traveling to/from Europe/Africa/Port Royal keep their plot coordinates (at the map edge) but should not interact with gameplay on that tile. A ship in transit could be "involved in combat" when enemies attacked the tile.
canDefend(): AddedisOnMap()check. WhilegetBestDefender()already filtered viaisOnMap(), the lower-levelcanDefend()did not — sogetNumDefenders()(used by AI movement decisions) and any direct callers ofcanDefend()would incorrectly count traveling units as defenders.canMoveInto()harbour and barracks space calculations looped through all plot units without checking travel state. Traveling ships/troops incorrectly consumed space, potentially blocking valid movement.Fixes #796
Test plan
🤖 Generated with Claude Code