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 elexis)

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)

commands.txt (6.6 KB ) - added by elexis 8 years ago.

Download all attachments as: .zip

Change History (8)

by elexis, 8 years ago

Attachment: commands.txt added

comment:1 by elexis, 8 years ago

Milestone: Alpha 21Backlog

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.

comment:2 by elexis, 7 years ago

Description: modified (diff)

Attached replay still bugs on most recent svn (a22 feature freeze time).

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


  1. About the question why that Seed wassn't defined to begin with in gamesetup.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


  1. Gamesetup.cpp should also always start with Seed, as that is also used to initialize the simulation as of Phab:rP18604. But I expect it to not be able to trigger the Simulation2.cpp warning, because CSimulation2::SetMapSettings(JS::HandleValue settings) is only called for random maps, and Gamesetup.cpp always sets it for random maps.

comment:3 by elexis, 7 years ago

Keywords: simple removed

comment:4 by elexis, 7 years ago

In 19718:

Fix all broken checks for the absent (random-mapgen+simulation) Seed property in the engine, refs #3965.
Hence display the originally intended useful warnings (instead of the characterless JS "Script value conversion check failed" warning and skipping the intended warnings).

comment:5 by elexis, 7 years ago

Component: UI & SimulationCore engine
Milestone: BacklogAlpha 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:6 by elexis, 7 years ago

In 19749:

Always set the Seed property from autostarted games, because it is required and used to initialize the RNG of the simulation following rP18604.

Differential Revision: https://code.wildfiregames.com/D592
Refs #4127, #3965

comment:7 by elexis, 7 years ago

Resolution: fixed
Status: newclosed
Summary: conversion check failed: v.isNumber() (got type undefined) on initMeaningless 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.

Note: See TracTickets for help on using tickets.