-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Simplify reuse of parent data cache when child gen has no override file #10614
Conversation
sim/dex.ts
Outdated
break; | ||
} | ||
if (dataType !== 'Pokedex' && childIsEmpty && !Scripts.init) { | ||
if (!dataCache[dataType] && !Scripts.init) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we still need the !== 'Pokedex' check, as counter-intuitive as that is.
When I add the deepFreeze line below,
// Execute initialization script.
if (Scripts.init) Scripts.init.call(this);
this.deepFreeze(dataCache);
npm test fails with
2116 passing (10s)
87 pending
1 failing
1) Burn
should reduce atk to 50% of its original value in Stadium:
TypeError: Cannot read properties of null (reading 'getStat')
at Context.<anonymous> (test/sim/misc/statuses.js:41:38)
at processImmediate (node:internal/timers:476:21)
When I add Dex.includeMods()
and Dex.includeModData()
to the very bottom of ./sim/dex.ts
, + logging, I get this
> mocha
loading data for fullpotential
reusing parent's data entry: fullpotential Rulesets
reusing parent's data entry: fullpotential FormatsData
reusing parent's data entry: fullpotential Items
reusing parent's data entry: fullpotential Learnsets
reusing parent's data entry: fullpotential Moves
reusing parent's data entry: fullpotential Natures
reusing parent's data entry: fullpotential Pokedex
reusing parent's data entry: fullpotential Conditions
reusing parent's data entry: fullpotential TypeChart
reusing parent's data entry: fullpotential PokemonGoData
loading data for gen1
loading data for gen2
loading data for gen3
loading data for gen4
loading data for gen5
loading data for gen6
loading data for gen7
loading data for gen8
reusing parent's data entry: gen8 Natures
reusing parent's data entry: gen8 Conditions
reusing parent's data entry: gen8 PokemonGoData
reusing parent's data entry: gen7 Learnsets
reusing parent's data entry: gen7 Natures
reusing parent's data entry: gen7 Conditions
reusing parent's data entry: gen7 TypeChart
reusing parent's data entry: gen7 PokemonGoData
reusing parent's data entry: gen6 Rulesets
reusing parent's data entry: gen6 Natures
reusing parent's data entry: gen6 PokemonGoData
reusing parent's data entry: gen5 Learnsets
reusing parent's data entry: gen5 Natures
reusing parent's data entry: gen5 PokemonGoData
reusing parent's data entry: gen4 Learnsets
reusing parent's data entry: gen4 Natures
reusing parent's data entry: gen4 TypeChart
reusing parent's data entry: gen4 PokemonGoData
reusing parent's data entry: gen2 Abilities
reusing parent's data entry: gen2 Natures
loading data for gen1jpn
reusing parent's data entry: gen1jpn Abilities
reusing parent's data entry: gen1jpn FormatsData
reusing parent's data entry: gen1jpn Items
reusing parent's data entry: gen1jpn Learnsets
reusing parent's data entry: gen1jpn Natures
reusing parent's data entry: gen1jpn Pokedex
TypeError: Cannot assign to read only property 'missingno' of object '#<Object>'
at ModdedDex.modData (redacted/dist/sim/dex.js:169:36)
at ModdedDex.init (redacted/dist/data/mods/gen1/scripts.js:56:25)
at ModdedDex.loadData (redacted/dist/sim/dex.js:537:20)
at ModdedDex.includeData (redacted/dist/sim/dex.js:455:10)
at ModdedDex.includeModData (redacted/dist/sim/dex.js:450:18)
When I add back the !== Pokedex
check, it works again.
Maybe the bug originates elsewhere, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I tested that on master + remove Pokedex check
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weirdly, if I then replace line 433's return dataObject[dataType];
with return Object.assign({}, dataObject[dataType]);
it then works again...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug is with Scripts.init
. This can be set by inheritance, although that makes no sense...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the inherited Scripts
is only read by line 547, at least during loadData
, since the const Scripts
variable on 490 reads a file from basePath
, and then lines 518 and 552 use that local variable.
Inheriting Script
in the first place is strange, I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aaaand now I get it. Yeah, that's the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, Scripts
gets require
d twice, so it's actually the same object as dataCache.Scripts
all along, and will pick up its copy of the init
function by inheritance. (Which is why my shallow clone hack worked.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Long ago, Scripts.init
was safely preserved in a different way. I don't know why that safety net got removed along they way.
Lines 81 to 84 in 6800e49
var BattleScripts = maybeScripts.BattleScripts; | |
if (!BattleScripts || typeof BattleScripts !== 'object') throw new TypeError("Exported property `BattleScripts`from `./data/scripts.js` must be an object except `null`."); | |
if (BattleScripts.init) Object.defineProperty(this, 'initMod', {value: BattleScripts.init, enumerable: false, writable: true, configurable: true}); | |
if (BattleScripts.inherit) Object.defineProperty(this, 'inheritMod', {value: BattleScripts.inherit, enumerable: false, writable: true, configurable: true}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it got "lost" as part of 2604780.
b882a68
to
9443991
Compare
New line 522 is the same as old line 518 but git likes to show it as being moved rather than lines 528-529/519-520.
PR #10611 means that we don't need to check for Pokedex in line 527/518 any more.