What good and bad parts have you found in the default RMMZ 0.9.5 codebase?

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
531
Reaction score
232
First Language
English
Primarily Uses
RMMV
I've actually tried that back in VXA, and it causes compatibility issues with any script adding unserializable properties to the game battler objects, so I've given up this approach in MV, as fixing those compatibility issues, if even possible at all, are all too complicated and convoluted(obviously the same problem can happen in MV and MZ as not everything in JavaScript can be stringified).
Um, I'm confused. If someone adds an unserializable property to the game battler object, it's going to cause way more problems than just this, right? The saved game format will break too, won't it? So why do you have to care about that possibility? If you find a plugin that does it, then report it as a bug.

Of course, I'd say that scripts/plugins shouldn't add unserializable properties to the game battler objects unless absolutely necessary, sometimes such additions are indeed called for,
I'm curious about what situations might actually require an unserializable property to be added to a game object.

I'm also curious what kinds of properties are actually unserializable? As far as I know, only functions can't be serialized in JSON, and those are normally in the prototype where the serializer doesn't even look.

and even if they're not, there's really not much we can do when some plugin developers insist to do so in cases totally not warranting such additions.
You can report it as a bug on the basis that it breaks saved games.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,749
Reaction score
912
First Language
Chinese
Primarily Uses
N/A
Um, I'm confused. If someone adds an unserializable property to the game battler object, it's going to cause way more problems than just this, right? The saved game format will break too, won't it? So why do you have to care about that possibility? If you find a plugin that does it, then report it as a bug.
I recall those scripts/plugins add those unserializable properties to the battler objects upon battle start, and remove them upon battle end(players can't save during battles after all), but it turns out that removing them right before making a deep copy and adding them back right after so during battles are just too complicated and convoluted for me(at least I've to preserve the states of those properties but I have no idea on how those properties work under the hood), at least back then, so I've given up the deep copy approach entirely.
As for what those unserailizable properties are, I've already forgot the exact details, as the last time I faced such issues are at least 5 years ago, but it's vaguely about storing some sprites on the battlers to support sideview battle systems(I'm really not sure if I've remembered it correctly) and those sprites have some unserializable stuffs there(normally sprites aren't saved after all).

Also, I don't know, but I just feel that making deep copy is generally expensive(especially when the whole window environment's cloned), and doing so just to prevent damage formula side effects from leaking out seems to be an overkill.
I think I'd rather let users write a notetag for each damage formula having side effects, and those notetag store the side effect free version of the damage formula :)
 
Last edited:

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
531
Reaction score
232
First Language
English
Primarily Uses
RMMV
I see. Well, I have two comments on that. First of all, if they don't matter for damage calculation, then making a deep copy that is missing those properties should be sufficient. Secondly, I'm not convinced it's correct to assume that players can't save during battles. True, by default it's not allowed, but are there actual reasons why it can't be allowed? The only problem I can think of for allowing it is that the battle manager isn't serialized...

Anyway, yeah, a deep copy is expensive, but in a language without constness, it's really the only way to enforce purity, I think. Cloning the entire environment probably isn't necessary for 99% of cases; if you clone just the variables, switches, and battler, it's probably not that expensive.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,749
Reaction score
912
First Language
Chinese
Primarily Uses
N/A
While it seems abrupt for me to suddenly talk about the bad parts in Scene_Battle, so I decided to do so anyway, as it's a very, very critical bug, although it's very, very hard to reproduce even if you know the root causes.
To reproduce it:
1. Play with Active TPBS
2. Select an actor with very low hp and be ready to input actions at any time
3. Right when an enemy starts to execute an action, immediately open the skill window for the actor that's about to die
4. Repeat 3 until the enemy action does hit and kill that actor
5. If you're making a command(attack, guard, confirm targets, select skill/item) right at the moment that actor just died, the game will just crash due to null BattleManager.inputtingAction() in Scene_Battle.prototype.commandAttack, Scene_Battle.prototype.commandGuard, Scene_Battle.prototype.onActorOk, Scene_Battle.prototype.onEnemyOk, Scene_Battle.prototype.onSkillOk, Scene_Battle.prototype.onItemOk and Scene_Battle.prototype.onSelectAction.

However, the existence of this bug is actually very understandable, because it's just so extremely rare, and it takes a veteran ATB system plugin developer having faced the very same bug before to even have a chance to grasp what's really going on under the hood.
While this also confirms that the RM devs aren't very familiar with implementing ATB systems, it's totally fine for me, as no one will be experienced on this before actually making 1 such system, and everyone has to start somewhere.

Unfortunately, there's really only 1 easy, simple and small way to fix the bug: For all those Scene Battle methods calling BattleManager.inputtingAction(), check whether it returns null and stop executing the rest of those methods if it's indeed null.
Any other fix will only lead to even more troubles
, because of the following sequence per frame:
1. The input window states(whether it's active and/or open) update -> Action execution effects update(on targets) -> Player input command update(what pressed keys are handled by which Scene_Battle methods)
2. If the action execution effects and player input command take place in the exact same frame, and the currently selected inputting actor dies because of those effects, there's no way to stop the Scene_Battle methods from entering invalid states(they need an existing inputting actor but that actor just died)
3. Therefore, the only way to fix the bug is to stop executing the rest of the method as soon as invalid states are detected, and in this case, it's to check whether BattleManager.inputtingAction() returns null
On a side note: This analysis demands you to reason about ATB system implementations in terms of frames in details, and inexperienced ATB system plugin developers will have a hard time doing this, so don't feel daunted if you can't follow ;)

Nevertheless, it's still a minefield waiting to explode, and I'm sure I won't be the only one stepped on this minefield in the default RMMZ codebase, so this bug should still be fixed before more and more users report bugs seemingly coming out of nowhere :)
 
Last edited:

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,749
Reaction score
912
First Language
Chinese
Primarily Uses
N/A
Bad parts:
Game_Actor.prototype.initSkills effectively hardcodes the assumptions that the actor level and skill learning levels are the only elements of deciding whether a skill should be learnt upon initializing the actor.
Fixing that can mean rewriting Game_Actor.prototype.initSkills into something like this:
JavaScript:
Game_Actor.prototype.initSkills = function() {
    this._skills = [];
    this.currentClass().learnings.forEach(this.initLearntSkill, this);
};
Game_Actor.prototype.initLearntSkill = function(learning) {
    if (!this.isInitLearntSkill(learning)) return;
    this.learnSkill(learning.skillId);
};
Game_Actor.prototype.isInitLearntSkill = function(learning) {
    return learning.level <= this._level;
};
Now all the plugin developers need to do is to edit Game_Actor.prototype.isInitLearntSkill in order to edit the aforementioned hardcoded assumption.

Game_Actor.prototype.releaseUnequippableItems uses an extremely ineffective and inefficient algorithm by looping over the same equip multiple times, and should be rewritten into something like this:
JavaScript:
Game_Actor.prototype.releaseUnequippableItems = function(forcing) {
    const slots = this.equipSlots();
    this._equips.forEach((equip, i) => {
        this.releaseUnequippableItem(forcing, equip, slots[i]);
    });
};
Game_Actor.prototype.releaseUnequippableItem = function(forcing, equip, slot) {
    if (!this.isUnequippableItem(equip.object(), slot)) return;
    this.onReleaseUnequippableItem(forcing, equip);
};
Game_Actor.prototype.isUnequippableItem = function(item, slot) {
    return item && (!this.canEquip(item) || item.etypeId !== slot);
};
Game_Actor.prototype.onReleaseUnequippableItem = function(forcing, equip) {
    if (!forcing) this.tradeItemWithParty(null, equip.object());
    equip.setObject(null);
};

Game_Actor.prototype.levelUpeffectively hardcodes the assumptions that the actor level and skill learning levels are the only elements of deciding whether a skill should be learnt upon actor level up.
Fixing that can mean rewriting Game_Actor.prototype.levelUp into something like this:
JavaScript:
Game_Actor.prototype.levelUp = function() {
    this._level++;
    this.currentClass().learnings.forEach(this.learnLevelUpSkill, this);
};
Game_Actor.prototype.learnLevelUpSkill = function(learning) {
    if (!this.isLearnLevelUpSkill(learning)) return;
    this.learnSkill(learning.skillId);
};
Game_Actor.prototype.isLearnLevelUpSkill= function(learning) {
    return learning.level === this._level;
};
Now all the plugin developers need to do is to edit Game_Actor.prototype.isLearnLevelUpSkill in order to edit the aforementioned hardcoded assumption.

Game_Actor.prototype.makeAutoBattleActions uses an extremely ineffective and inefficient algorithm by evaluating all autobattle actor actions for each action slot.
While the evaluation result is random, so the best action for action slot 1 might not be the best for action slot 2, the same actually applies to the very evaluation itself - While an action's evaluated to be the best for an action slot in the input phase, the same might not hold in the execution phase.
Therefore, with the random nature of the evaluation, it's inherently inaccurate, and can only be as most used by an approximate estimation.
With the current implementation, it does increase variety among different action slots of the same autobattle actor, but as the goal of the evaluation is to maximize the chance for the autobattle actors to execute actions with the maximum damages, the implementation should've been just evaluating all actions of each autobattle actor once, then assign that action to all action slots of that actor.

It's because, assuming that there's a plugin adding an autobattle command that forces all actors to be in autobattle for the current turn, and there are 4 actors, each having 100 usable skills and 5 action slots, there will be 2000 evaluations under the current implementation, which is 2000 JavaScript eval calls as the damage formulae are to be run that way.
Needless to say, 2000 JavaScript eval calls at the same time will kill even the most powerful PC, so the variety provided by the current implementation isn't worth such an obscene performance penalty.
On the other hand, with the same evaluation action assigned to all action slots of the same autobattle actor, only 400 eval calls will be needed, even though it's still a nightmare to work with(but is much better already).
So Game_Actor.prototype.makeAutoBattleActions should be rewritten into something like this:
JavaScript:
Game_Actor.prototype.makeAutoBattleActions = function() {
    this.setAutoBattleActs();
    this.setActionState("waiting");
};
Game_Actor.prototype.setAutoBattleActs = function() {
    const action = this.autoBattleAct();
    for (let i = 0, numActions = this.numActions(); i < numActions; i++) {
        this.setAction(i, action);
    }
};
Game_Actor.prototype.autoBattleAct = function() {
    let maxValue = Number.MIN_VALUE, bestAct;
    this.makeActionList().forEach(action => {
        const value = action.evaluate();
        if (value <= maxValue) return;
        [maxValue, bestAct] = [value, action];
    });
    return bestAct;
};

Game_Actor.prototype.executeFloorDamage hardcodes the floor damage to use the formula of Math.min(Math.floor(this.basicFloorDamage() * this.fdr), this.maxFloorDamage()), and some plugins will want to change that, so Game_Actor.prototype.executeFloorDamage should be rewritten into something like this:
JavaScript:
Game_Actor.prototype.executeFloorDamage = function() {
    const realDamage = this.realFloorDamage();
    this.gainHp(-realDamage);
    if (realDamage > 0) {
        this.performMapDamage();
    }
};
Game_Actor.prototype.realFloorDamage = function() {
    const floorDamage = Math.floor(this.basicFloorDamage() * this.fdr);
    return Math.min(floorDamage, this.maxFloorDamage());
};

Bad parts:
Game_Enemy.prototype.selectAllActions effectively hardcodes the assumptions that skills with rating being max - 3 or lower will be discarded, where max is the rating of the usable skill with the highest rating.
To fix that, Game_Enemy.prototype.selectAllActions can be rewritten into something like this:
JavaScript:
Game_Enemy.prototype.selectAllActions = function(actionList) {
    const ratingZero = this.ratingZero(actionList);
    actionList = actionList.filter(({ rating }) => rating > ratingZero);
    for (let i = 0; i < this.numActions(); i++) {
        const selectedAct = this.selectAction(actionList, ratingZero);
        this.action(i).setEnemyAction(selectedAct);
    }
};

Game_Enemy.prototype.ratingZero = function(actionList) {
    return Math.max(...actionList.map(({ rating }) => rating)) - 3;
};
Now plugin developers can at least change Game_Enemy.prototype.ratingZero to change the aforementioned hardcoded assumptions.

Bad parts:
There's just 1 - The scripts in events are always reevaluated each time, instead of being cached into new functions, thus turning repeated eval into repeated function calls.
With unnecessarily repeated eval, there can be FPS drop and/or sudden lag if there are many script events to be run in a short time, because eval is perhaps one of the most expensive native JavaScript operations.
Of course, caching those scripts into new functions does have a problem - increased memory usage, especially when there are many script events.
Therefore, Game_Interpreter should've a flag to let RMMZ users decide whether the scripts should be cached into new functions or just reevaluated every time.
For instsance, the script event command can be rewritten into something like this:
JavaScript:
Game_Interpreter.prototype.command355 = function() {
    var script = this.currentCommand().parameters[0] + '\n';
    while (this.nextEventCode() === 655) {
        this._index++;
        script += this.currentCommand().parameters[0] + '\n';
    }
    if (Game_Interpreter.IS_CACHE_SCRIPTS) {
        if (!Game_Interpreter._cachedScripts.has(script)) {
            Game_Interpreter._cachedScripts.set(script, new Function(script));
        }
        Game_Interpreter._cachedScripts.get(script).call(this);
    } else eval(script);
    return true;
};
And the same can apply to Game_Interpreter.prototype.command111 and Game_Interpreter.prototype.command122.
On a side note: Actually the same applies to evaluating damage formula in Game_Action ;)

Parts unsure whether it's bad:
The naming of many Game_Interpreter methods and variables, as well as the hardcoded constant values, are magic numbers, like Game_Interpreter.prototype.command111 being conditional branch, params[0] being switch for value 0, causing the readability of those codes to be drastically reduced.
However, as these parts are interacting with the RMMZ editor codes directly, and I don't know if those names are due to the RMMZ editor code constraints, so I'm not sure whether it's really a bad part :)
 

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
531
Reaction score
232
First Language
English
Primarily Uses
RMMV
Game_Actor.prototype.releaseUnequippableItems uses an extremely ineffective and inefficient algorithm by looping over the same equip multiple times
While it's nice to make it more efficient, I don't think it's going to make any difference in practice. When looping over 4-10 items, it won't make much difference whether you do it once, twice, or thrice. Maybe if someone made a game with 10,000 equipment slots, then it would matter.

Unless you meant it's looping over all database items multiple times.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,749
Reaction score
912
First Language
Chinese
Primarily Uses
N/A
While it's nice to make it more efficient, I don't think it's going to make any difference in practice. When looping over 4-10 items, it won't make much difference whether you do it once, twice, or thrice. Maybe if someone made a game with 10,000 equipment slots, then it would matter.

Unless you meant it's looping over all database items multiple times.
In that case, yes, it doesn't matter much in terms of performance, but the general side effects of using an obviously ineffective and inefficient algorithm is also reduced readability, because you'll wonder why such an ineffective and inefficient algorithm's used.
For instance, in this case, I've to dry run the codes many, many times, which used me several minutes, just to be sure that the nested loops are actually unnecessary.
So sometimes coming up with a more effective and efficient algorithm isn't just about performance, but also readability, even though it's not always the case(as sometimes the most effective and efficient algorithm can be unreadable as hell).
 

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
531
Reaction score
232
First Language
English
Primarily Uses
RMMV
That's true, sometimes a more efficient algorithm is more readable. On the other hand, sometimes a less efficient algorithm is more reasonable. I haven't looked at the original code in this case so I can't comment on the readability.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,749
Reaction score
912
First Language
Chinese
Primarily Uses
N/A
Now let's talk about some bad parts I've found in Game_Map, Game_CharacterBase and Game_Character:
roundXWithDirection has inherently duplicated codes from xWithDirection, meaning that those codes represent identical knowledge, because roundXWithDirection is supposed to round the result from xWithDirection, so roundXWithDirection should be rewritten into something like this:
Code:
Game_Map.prototype.roundXWithDirection = function(x, d) {
    return this.roundX(this.xWithDirection(x, d));
};
The same applies to roundYWithDirection.

While the current implementation does add more flexibility for some plugins, it also leads to needless duplicated changes for some other plugins, with some pixel movement plugins that changes the implementation of xWithDirection and yWithDirection, causing roundXWithDirection and roundYWithDirection to be unnecessarily changed as well.
For plugins having to change the definition of roundXWithDirection from rounding the result from xWithDirection to something else, roundXWithDirection is going to be rewritten anyway, so my proposed change should do way more good than harm.

Right now, moveStraight has included too many concrete details that should've been abstracted away from it, so it should be written into something like this:
Code:
Game_CharacterBase.prototype.moveStraight = function(d) {
    this.setMovementSuccess(this.canPass(this._x, this._y, d));
    if (this.isMovementSucceeded()) return this._onMoveStraightSuc(d);
    this._onMoveStraightFail(d);
};
Game_CharacterBase.prototype._onMoveStraightSuc = function(d) {
    this.setDirection(d);
    this._updateStraightXY(d);
    this.increaseSteps();
};
Game_CharacterBase.prototype._updateStraightXY = function(d) {
    this._x = $gameMap.roundXWithDirection(this._x, d);
    this._y = $gameMap.roundYWithDirection(this._y, d);
    this._realX = $gameMap.xWithDirection(this._x, this.reverseDir(d));
    this._realY = $gameMap.yWithDirection(this._y, this.reverseDir(d));
};
Game_CharacterBase.prototype._onMoveStraightFail = function(d) {
    this.setDirection(d);
    this.checkEventTriggerTouchFront(d);
};
This will make writing some plugins, like pixel movement, much easier, simpler and smaller tasks, as just _onMoveStraightSuc, instead of the whole moveStraight, needs to be touched.

Similarly, moveDiagonally should be rewritten into something like this:
Code:
Game_CharacterBase.prototype.moveDiagonally = function(horz, vert) {
    const canPass = this.canPassDiagonally(this._x, this._y, horz, vert);
    this.setMovementSuccess(canPass);
    if (this.isMovementSucceeded()) this._onMoveDiagonallySuc(horz, vert);
    if (this._direction === this.reverseDir(horz)) this.setDirection(horz);
    if (this._direction === this.reverseDir(vert)) this.setDirection(vert);
};
Game_CharacterBase.prototype._onMoveDiagonallySuc = function(horz, vert) {
    this._updateDiagonalXY(horz, vert);
    this.increaseSteps();
};
Game_CharacterBase.prototype._updateDiagonalXY = function(horz, vert) {
    this._x = $gameMap.roundXWithDirection(this._x, horz);
    this._y = $gameMap.roundYWithDirection(this._y, vert);
    this._realX = $gameMap.xWithDirection(this._x, this.reverseDir(horz));
    this._realY = $gameMap.yWithDirection(this._y, this.reverseDir(vert));
};
So plugins like pixel movement just have to touch _onMoveDiagonallySuc instead of the whole moveDiagonally.

Like Game_BattlerBase and Game_Battler, the existence of an abstract class having just 1 child, which is itself another abstract class, almost always indicates that the class has taken too many responsibilities, and the former abstract class is an attempt to prevent the latter one from being a god class.
Again, it's not a good use of inheritance, which is supposed to be an is-a relationship, because it clearly makes no sense to state that Game_Character is a Game_CharacterBase, unlike "Game_Player is a Game_Character", which is perfectly fine.

So, composition over inheritance should be applied to Game_Character instead of using Game_CharacterBase at all, implying that the responsibility set included by Game_Character and Game_CharacterBase should've been broken down into several helper objects, like the following:
1. Game_CharacterMovement(position, move speed, move frequency, jump, stop, dash, etc)
2. Game_CharacterCollision(can pass, is map passable, is collided with characters, etc)
3. Game_CharacterAnime(walk anime, step anime, opacity, balloon, animation, blend mode, etc)
4. Game_CharacterMoveRoute(move route, move toward, move away from, turn toward, turn away, direction, etc)
While I'm not very familiar with Game_CharacterBase nor Game_Character so I'm not sure how these 4 helper objects would interact with each other, I think that the very existence of these helper objects replacing the Game_CharacterBase will lead to an even more organized and structured Game_Character :)
 
Last edited:

Users Who Are Viewing This Thread (Users: 0, Guests: 1)

Latest Threads

Latest Posts

Latest Profile Posts

Yesterday I made my first step towards eating more healthily.
I saw candy on discount and did not buy it.
"They yearn for what they fear for."
I always told my DA fans how much I hate slot machines. They're fine in games as a risk-and-reward system. But when you're spending REAL MONEY in a Vegas casino to try and hit the jackpot (which very, very few people will), it can really hurt your budget. Gambling is a bad habit, and I don't like wasting my money on a slim chance. Go to Vegas for the experience, not the jackpot.
Took the kids to a corn maze. They gave us a map and had lights at certain points in the maze. Not overwhelming... or underwhelming... just... whelming.
Okay, vacuuming fruit flies out of the air is surprisingly effective.

Forum statistics

Threads
104,392
Messages
1,006,061
Members
135,925
Latest member
Gubokkero1
Top