Opened 8 years ago
Closed 7 years ago
#3965 closed defect (fixed)
Meaningless error if the Seed was not set
Reported by: | elexis | Owned by: | |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 22 |
Component: | Core engine | Keywords: | |
Cc: | Patch: |
Description (last modified by )
The attached replay (r18136) triggers a warning before the first turn, even before loading the random-map-scripts:
WARNING: JavaScript warning: Script value conversion check failed: v.isNumber() (got type undefined)
Fixing this will likely be trivial. The hard part is figuring out in which file the warning is triggered in the first place. Adding some warn()
calls to simulation files should lead there.
Attachments (1)
Change History (8)
by , 8 years ago
Attachment: | commands.txt added |
---|
comment:1 by , 8 years ago
Milestone: | Alpha 21 → Backlog |
---|
comment:2 by , 7 years ago
Description: | modified (diff) |
---|
Attached replay still bugs on most recent svn (a22 feature freeze time).
- About the place where the warning occurs:
Added LOGWARNINGs all over the place, still didn't find it.
Decided to search for the warning string and added a breakpoint where it's thrown. Thus got this stack:
Thread #1 [pyrogenesis] 8601 [core: 0] (Suspended : Breakpoint) ScriptInterface::FromJSVal<unsigned int>() at ScriptConversions.cpp:93 0x5c045d ScriptInterface::GetProperty<unsigned int>() at ScriptInterface.h:531 0x4782fe CSimulation2::SetMapSettings() at Simulation2.cpp:797 0x4782fe CMapReader::LoadRMSettings() at MapReader.cpp:1,235 0x6dffc3 MemFunThunk<CMapReader>() at LoaderThunks.h:60 0x6dfaa4 LDR_ProgressiveLoad() at Loader.cpp:228 0x5f1479 LDR_NonprogressiveLoad() at Loader.cpp:318 0x5f1835 CReplayPlayer::Replay() at Replay.cpp:179 0x5c94a2 RunGameOrAtlas() at main.cpp:519 0x43058e main() at main.cpp:623 0x422747
So it occurs in L797 of Simulation2.cpp
, which is:
if (!m->m_ComponentManager.GetScriptInterface().GetProperty(m->m_MapSettings, "Seed", seed)) LOGWARNING("CSimulation2::SetInitAttributes: No seed value specified - using %d", seed);
So we can see a check that doesn't actually work was added and should use HasProperty
.
Now the fun thing is that this might occur with many hundreds of GetProperty
calls, so someone funny could check all occurances or write unit tests for all occurances.
An alternative would be adding a Has
check to Scriptinterface->GetProperty
, but that seems wrong because we get the same warning in JS and actually do want to have that warning instead of silently returning undefined, don't we?
If we want to remove that warning altogether, reopen the ticket.
- About the question why that
Seed
wassn't defined to begin with ingamesetup.js
:
It must have been some kind of persist-match-settings bug or map-type changed bug.
Before Phab:rP18604, the Seed
wasn't always set on launchGame
but only on loadPersistMatchSettings
and selectMapType
. Still don't see a way how this could have occured:
https://code.wildfiregames.com/source/0ad/browse/ps/trunk/binaries/data/mods/public/gui/gamesetup/gamesetup.js;18127
Gamesetup.cpp
should also always start withSeed
, as that is also used to initialize the simulation as of Phab:rP18604. But I expect it to not be able to trigger theSimulation2.cpp
warning, becauseCSimulation2::SetMapSettings(JS::HandleValue settings)
is only called for random maps, andGamesetup.cpp
always sets it for random maps.
comment:3 by , 7 years ago
Keywords: | simple removed |
---|
comment:5 by , 7 years ago
Component: | UI & Simulation → Core engine |
---|---|
Milestone: | Backlog → Alpha 22 |
(1) committed above. And we don't want to mute the warning as that comment in ScriptConversions.cpp
throwing the warning states:
// Implicit type conversions often hide bugs, so warn about them #define WARN_IF_NOT(c, v) STMT(if (!(c)) { JS_ReportWarning(cx, "Script value conversion check failed: %s (got type %s)", #c, InformalValueTypeName(v)); })
(2) only god knows why
(3) Phab:D592
comment:7 by , 7 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Summary: | conversion check failed: v.isNumber() (got type undefined) on init → Meaningless error if the Seed was not set |
So the error occured the first time with rP18136 and can't occur anymore as of rP18297 afaics.
The analog Gamesetup.cpp
issue was addressed by the two commits above.
Still broken with r18722. Someone should place warn("foo") to some JS files like random map script initialization and some
debug_printf
calls to the C++ files to figure out where that simulation bug occurs.