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

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
While I've only read the core and managers parts of the default RMMZ 0.9.5 codebase, the following's some of the good and bad parts I've found(by no way exhaustive and in my opinion of course):
Core -
1. The adaptation of Pixi v5.2.4 in MZ from v4.5.4 in MV(performance boost)
2. The use of GLSL in MZ instead of doing the same by the JavaScript in MV(performance boost)
3. The existence of Graphics.FSPCounter and Video classes in MZ instead of having so many functionality included by Graphics in MV(significantly reducing complexity of Graphics class while only slighlty increasing complexity for trying to keep the whole codebase inside the head)
4. The existence of Utils.checkRMVersion in MZ having no equivalent in MV(making version checking easier, simpler and smaller tasks)
5. The removal of many low level implementation details from many core classes in MZ unlike many MV codes that seems to be doing the Pixi works(To be fair, some of those low level implementations come from the fact that MV natively supports Canvas mode while MZ doesn't natively support this outdated mode anymore, which is another good parts in MZ)
6. The removal of the Html5Audio class in MZ, which is mostly unused in most MV projects anyway, and nowsdays only IE doesn't support the Web Audio API, and it's unlikely that anyone will still use IE, let alone for playing MZ games(might be bad for those minority though)

Managers -
1. The existence of FontManager in MZ extracting the similar functionality from Graphics in MV(significantly reducing complexity of Graphics class while only slighlty increasing complexity for trying to keep the whole codebase inside the head)
2. The existence of ColorManager in MZ extracting what should be independent functionality from Window_Base in MV(now getting window colors doesn't have to do weird hacks like new Window_Base().normalColor() and the codebase makes it clear that windows aren't the only ones that are supposed to use window colors)
3. No more status window injection from Scene_Battle to BattleManager in MZ, unlike what's done in MV(that MV design flaw causes inherently duplicated codes among refreshStatus in BattleManager and Scene_Battle, as they're essentially the identical knowledge just with different timings, meaning that MV should've injected Scene_Battle.prototype.refreshStatus into the BattleManager instead)
4. The existence of BattleManager.updatePhase in MZ extracted from BattleManager.update in MV(this makes adding/editing/removing battle phases much, much easier, simpler and smaller tasks in MZ as BattleManager.updateEvent has side effects that should only occur at most once per frame)
5. The existence of _currentActor in BattleManager in MZ(at least a slight performance boost and boilerplate reduction)
1. No native TypeScript support(even though @nio kasgami is burning his/her life to help plugin developers write TypeScript plugins), which further tightens the plugin development feedback loop by letting the compiler complain about careless but common mistakes right after they're made instead of in run time, and helps those writing automated tests to test their plugins by setting up test fixtures and mock dependencies, with the additional information from TypeScript so they'll always know exactly the data type of the dependency to be mocked(also, some tests are asserting a variable/parameter/arguement's of the right data type, and the importance of these tests will be much, much lower with TypeScript)

2. Not fully utilizing ES6 constructs in many places actually improving codebase quality(it's different from forcing the whole codebase into the ES6 standard though), and nowadays most JavaScript programmers know ES6 as it's long become the mainstream years ago, and actually there will be more not knowing ES5(like some of those learnt JavaScript after ES6 becomes the mainstream) than those not knowing ES6(like some of those learning JavaScript for MV and nothing more)

3. It's very, very challenging to write MZ plugins with testability in mind, let alone a fully automated test suite with almost entirely of meaningful tests, high code and state coverage, as the default RMMZ codebase isn't very testable in itself(I'm not demanding ECS with TDD though), mainly because of the fact that many classes(many of them should be broken down into several smaller classes in my opinion) have too many interdependent shared mutable states, which should be broken down into smaller and more independent pieces(right now it's hard to mock or setup test fixtures effectively and efficiently without a commercial level testing framework, and the need of these things usually means the testability of the codebase's far from ideal), and shoehorning testable codes into such a codebase will probably lead to tons of unreadable codes(having too many seams just to rescue testability will make codes hard to reason and dry-run in details), tautological tests or those with unclear implications when they fail(i.e., you can't immediately tell which parts of nontrivial business logic's compromised), meaning that the ROI of automated test suites of plugins is generally too low to be worth, but manually testing plugins clearly undesirable for those plugin developers and users either

4. No automated test suite(public to plugin developers) in the default RMMZ codebase, causing plugin developers to have one less tool to help them catch unknowing functionality breaks in the default RMMZ codebase and/or compatibility issues steming from plugin A relying on some assumptions in the default RMMZ codebase and plugin B has broken that unknowingly(I know I'm asking too much here, but still it'd be nice if plugin developers do have access to the automated test suite in the default RMMZ codebase), and sometimes such an automated test suite can even help plugin users report bugs/compatibility issues more effectively and efficiently

5. Still using rmmz_core.js, rmmz_managers.js, rmmz_objects.js, rmmz_scenes.js, rmmz_sprites.js and rmmz_windows.js instead of core/Graphics.js, core/Input.js, managers/BattleManager.js, managers/DataManager.js, etc, causing those files being too big to navigate effectively nor efficiently, and any decent IDE lets you search across all files in the same folder(including those in subfolders), so breaking those big files into smaller ones grouped by folders should do way more good than harm
1. The way Graphics._onKeyDown is rewritten has effectively hardcoded F2, F3 and F4 as switching FPS counter, switching stretch mode and switching full screen respectively, and there can be some plugins(like those letting players bind the keys themselves) that want to change these functionality to be triggered by some other keys, so that function should be rewritten to at least something like this:
JavaScript:
Graphics._onKeyDown = function(event) {
    if (!event.ctrlKey && !event.altKey) {
        const { keyCode } = event;
        if (this._switchFPSCounterKey(keyCode)) {
                event.preventDefault();
                this._switchFPSCounter();
        } else if (this._switchStretchModeKey(keyCode)) {
                event.preventDefault();
                this._switchStretchMode();
        } else if (this._switchFullScreenKey(keyCode)) {
                event.preventDefault();
                this._switchFullScreen();
        }
    }
};
Instead of just hardcoding 113(F2), 114(F3) and 115(F4) there.

2. There's no way to change the XHR in Bitmap.prototype._startDecrypting without rewriting that instance method, and fixing that's easy, simple and small task:
Code:
Bitmap.prototype._startDecrypting = function() {
    this._newDecryptingXhr().send();
}
Clearly there's no reason to exclude the option to let plugins change that XHR, especially when including it costs next to nothing.

3. It's clear that Tilemap.Renderer.prototype._createShader doesn't want plugin developers to change the vertex and fragment GLSL, otherwise there's no reason for it to be written this way, but there are and/or will be plugin developers being familiar with GLSL and wanting to change the GLSL, so that instance method should be rewritten into something like this:
JavaScript:
Tilemap.Renderer.prototype._createShader = function()
    const vertexSrc = this._shaderVertexSrc();
    const fragmentSrc = this._shaderFragmentSrc();
    return new PIXI.Shader(PIXI.Program.from(vertexSrc, fragmentSrc), {
        uSampler0: 0,
        uSampler1: 1,
        uSampler2: 2,
        uProjectionMatrix: new PIXI.Matrix()
    });
};
Again, such a rewrite costs next to nothing.

4. TilingSprite.prototype._refresh has some redundant allocations, because if texture's falsy or its base texture's falsy, the cloned frame won't be used at all, so that instance method should be rewritten into at least something like this:
JavaScript:
TilingSprite.prototype._refresh = function() {
    const texture = this.texture;
    if (texture) {
        if (texture.baseTexture) {
            try {
                const frame = this._frame.clone();
                if (frame.width === 0 && frame.height === 0 && this._bitmap) {
                    frame.width = this._bitmap.width;
                    frame.height = this._bitmap.height;
                }
                texture.frame = frame;
            } catch (e) {
                texture.frame = new Rectangle();
            }
        }
        texture._updateID++;
    }
};
While such redundant allocations don't have much performance impact as that instance method's not a hotspot, it's still slightly sloppy codes nonetheless.

5. WindowLayer.prototype.render doesn't let plugins change how each child's rendered, and can become a serious obstacle when Pixi releases a new version(stable or not) and some plugins want to test it out before the default RMMZ codebase adapts to that new version, so that instance method should be rewritten into at least something like this:
JavaScript:
WindowLayer.prototype.render = function render(renderer) {
    if (!this.visible) {
        return;
    }

    const graphics = new PIXI.Graphics();
    const gl = renderer.gl;
    const children = this.children.clone();

    renderer.framebuffer.forceStencil();
    graphics.transform = this.transform;
    renderer.batch.flush();
    gl.enable(gl.STENCIL_TEST);

    while (children.length > 0) {
        const win = children.pop();
        if (win._isWindow && win.visible && win.openness > 0) {
            this._renderChild(renderer, graphics, gl, win);
        }
    }

    gl.disable(gl.STENCIL_TEST);
    gl.clear(gl.STENCIL_BUFFER_BIT);
    gl.clearStencil(0);
    renderer.batch.flush();

    for (const child of this.children) {
        if (!child._isWindow && child.visible) {
            child.render(renderer);
        }
    }

    renderer.batch.flush();
};
Now changing how each child's rendered just need to change WindowLayer.prototype._renderChild.

6. WebAudio.prototype._startXhrLoading has the same issues as Bitmap.prototype._startDecrypting and the similar cures:
JavaScript:
WebAudio.prototype._startXhrLoading = function(url) {
    this._newLoadingXhr(url).send();
};

7. Input still doesn't have an API to detect whether a key's just released in a given frame, and this API's very, very useful to some plugins triggering some events when a key's just released(I'm still writing 1 such MV plugin and I've to add the API into Input myself).

8. Input._onKeyDown hardcodes the numlock key to clear all input states, and some plugins might want to change that(perhaps for similar reasons to the issues in Graphics._onKeyDown), so that static function should be rewritten into at least something like this:
JavaScript:
Input._onKeyDown = function(event) {
    if (this._shouldPreventDefault(event.keyCode)) {
        event.preventDefault();
    }
    if (this._shouldClear(event)) {
        this.clear();
    }
    const buttonName = this.keyMapper[event.keyCode];
    if (buttonName) {
        this._currentState[buttonName] = true;
    }
};

9. Graphics.pageToCanvasX(eventTouch.pageX) and Graphics.pageToCanvasY(eventTouch.pageY) are needlessly repeated in many functions in TouchInput, as those duplications are inherently identical knowledge - Converting the event/touch page X and Y into the canvas X and Y, so these redundant codes can be removed by something like this:
JavaScript:
TouchInput._onLeftButtonDown = function(event) {
    const xy = this._pageToCanvasXY(event);
    if (Graphics.isInsideCanvas(...xy)) {
        this._mousePressed = true;
        this._pressedTime = 0;
        this._onTrigger(...xy);
    }
};

TouchInput._onRightButtonDown = function(event) {
    const xy = this._pageToCanvasXY(event);
    if (Graphics.isInsideCanvas(...xy)) {
        this._onCancel(...xy);
    }
};

TouchInput._onMouseMove = function(event) {
    const xy = this._pageToCanvasXY(event);
    if (this._mousePressed) {
        this._onMove(...xy);
    } else if (Graphics.isInsideCanvas(...xy)) {
        this._onHover(...xy);
    }
};

TouchInput._onMouseUp = function(event) {
    if (event.button === 0) {
        this._mousePressed = false;
        this._onRelease(...this._pageToCanvasXY(event));
    }
};

TouchInput._onTouchStart = function(event) {
    for (const touch of event.changedTouches) {
        const xy = this._pageToCanvasXY(touch);
        if (Graphics.isInsideCanvas(...xy)) {
            this._screenPressed = true;
            this._pressedTime = 0;
            if (event.touches.length >= 2) {
                this._onCancel(...xy);
            } else {
                this._onTrigger(...xy);
            }
            event.preventDefault();
        }
    }
    if (window.cordova || window.navigator.standalone) {
        event.preventDefault();
    }
};

TouchInput._onTouchMove = function(event) {
    for (const touch of event.changedTouches) {
        this._onMove(...this._pageToCanvasXY(touch));
    }
};

TouchInput._onTouchEnd = function(event) {
    for (const touch of event.changedTouches) {
        this._screenPressed = false;
        this._onRelease(...this._pageToCanvasXY(touch));
    }
};

TouchInput._pageToCanvasXY(eventTouch) {
    return [
        Graphics.pageToCanvasX(eventTouch.pageX),
        Graphics.pageToCanvasY(eventTouch.pageY)
    ];
}
And not keeping thing DRY here can lead to changes of the aforementioned conversions to have troubles involving finding all the places using those conversions.
1. DataManager.loadDataFile has the same issues as Bitmap.prototype._startDecrypting and the similar cures:
JavaScript:
DataManager.loadDataFile = function(name, src) {
    window[name] = null;
    this._loadDataFileXhr(name, src).send();
}

2. BattleManager.invokeAction hardcodes the assumption that cnt and mrf will always be used when applying an action to its targets, which is simply false for some plugins setting some special cases to bypass them, and asking those plugins to change the value of cnt and mrf in those cases can create compatibility issues for some other plugins using cnt and mrf for anything other than their intended uses, so that static function should be rewritten into at least something like this:
JavaScript:
BattleManager.invokeAction = function(subject, target) {
    this._logWindow.push("pushBaseLine");
    if (this._isInvokeCnt(target)) {
        this.invokeCounterAttack(subject, target);
    } else if (this._isInvokeMrf(target)) {
        this.invokeMagicReflection(subject, target);
    } else {
        this.invokeNormalAction(subject, target);
    }
    subject.setLastTarget(target);
    this._logWindow.push("popBaseLine");
};
Now bypassing cnt and mrf can be done by changing BattleManager._isInvokeCnt and BattleManager._isInvokeMrf, all without having to change the cnt and mrf values themselves.

3. BattleManager.onEscapeFailure hardcodes the escape ratio increment upon failed party escape attempts as 0.1, and there will definitely be plugins wanting to change that, so that static function should be rewritten into at least something like this:
JavaScript:
BattleManager.onEscapeFailure = function() {
    $gameParty.onEscapeFailure();
    this.displayEscapeFailureMessage();
    this._escapeRatio += this._escRatioIncrement();;
    if (!this.isTpb()) {
        this.startTurn();
    }
};
Now changing the escape ratio increment just involves changing BattleManager._escRatioIncrement.

4. BattleManager.changeCurrentActor uses an algorithm that's less readable and performant as it could be, and solving these issues can be done by rewriting that static function into at least something like this:
JavaScript:
BattleManager.changeCurrentActor = function(forward) {
    this._currentActor = this._newCurActor_(forward);
    this.startActorInput();
};

BattleManager._newCurActor_ = function(forward) {
    const members = $gameParty.battleMembers();
    const iIncrement = forward ? 1 : -1;
    let currentI = members.indexOf(this._currentActor);
    for (;;) {
        currentI += iIncrement;
        const actor = members[currentI];
        if (!actor) return null;
        if (actor.canInput()) return actor;
    }
};
It's because the original function access indexOf every time in the loop, leading to O(n ^ 2) algorithmic complexity, while this rewritten version just access it once, leading to O(n) algorithmic complexity.
While the performance impact doesn't matter in this case as n is always small the a large n itself will create even greater performance issues than what's created in this static function, it's still slightly sloppy codes nonetheless.
Also, the real intention of BattleManager.changeCurrentActor is to examine the next/prior actors(next is forward is true and prior if otherwise) to find an inputable actor, so the rewritten version can reflect that intention more accurately, thus increasing code readability.

5. SceneManager.onKeyDown has the similar issues as that in Graphics._onKeyDown, and that static function can be rewritten into something like this:
JavaScript:
SceneManager.onKeyDown = function(event) {
    if (!event.ctrlKey && !event.altKey) {
        const { keyCode } = event;
        if (this._reloadGameKey(keyCode)) return this.reloadGame();
        if (this._showevToolsKey(keyCode)) return this.showDevTools();
    }
};
There are actually a lot more bad parts I've found, but they're rather minor, at least compared to what I've included here, and I don't want this post to be just overwhelmed with all the bad parts, thus giving the wrong impression that the default RMMZ codebase's just an utterly pathetic mess and hurt the sales before it's even released, so I've also emphasized the good parts to make thing more fair.

I'd like to know what you've found about the good and bad parts in the default RMMZ 0.9.5 codebase as well, and I hope we can make a collection here to praise the MZ devs on one hand, and request them to make the codebase even better on the other.
Of course, with just 10 days before launch, it's just impossible to fix all these issues, but I still hope at least the most severe ones will be noticed and fixing them several days/weeks after release's still way better than nothing :)

P.S.: I hope the MZ devs will eventually read this, as I think the MZ improvements, while already quite drastic and in the right directions, can have some further works as additional quality of lives for many plugin developers, and I'm opening this post as a hobbyist, who needs a high quality codebase to work effectively and efficiently, so I think the risks of scaring off many hobbyists isn't so high that any more significant improvements will only do more harm than good, not to mention that not all hobbyists will remain hobbyists forever, and a very, very high quality codebase can help them grow.
 
Last edited:

nio kasgami

VampCat
Veteran
Joined
May 21, 2013
Messages
8,945
Reaction score
3,029
First Language
French
Primarily Uses
RMMV
Nice thread @DoubleX ! and thanks for recognize my efforts for the typescript definitions files....Indeed it's a lots of work and the Manager is mostly done as well (ALthough I might ask your assistance for the StorageManager typing since I am fairly not familiar with Promise)

For a good thing :

A LOT of small little QOL change such the Scene_Message which avoid a LOTS of redundancy, and the color manager , etc as you mentioned. Also Saving seem to Zip file now and is async / promise based which is quite an interesting thing? (if you look the DataManager it's all async based)

for the bad thing which is the worst of them all....

Window_BattleLog :rtear:
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
I haven't read the Window_BattleLog in MZ yet, but I just hope that it's not largely the same as that in MV, which is at least perhaps the most misleading class(those really thinking that it's just a battle log will be in a world of hurt very quickly) in MV lol
 

nio kasgami

VampCat
Veteran
Joined
May 21, 2013
Messages
8,945
Reaction score
3,029
First Language
French
Primarily Uses
RMMV
I am not 100% familliar with it but it does look the same...

I just pretend it doesn't exist ... :rtear:

HONESTLY I would separate EVERY battle logic from the Window_BattleLog and shove it into a : Game_BattleFlow.

which is WAY easier to fix IMO
 

Tsukihime

Veteran
Veteran
Joined
Jun 30, 2012
Messages
8,553
Reaction score
3,789
First Language
English
Is window battlelog that bad? Only couple things seem out of place to me is showing the skill apply animation. Everything else is still largely "battle log" stuff.
 

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
523
Reaction score
230
First Language
English
Primarily Uses
RMMV
While I've only read the core and managers parts of the default RMMZ 0.9.5 codebase, the following's some of the good and bad parts I've found(by no way exhaustive and in my opinion of course):
Well, it seems to me that none of the bad parts are really ground-breakingly terrible, so I suppose that's good to know.

I actually don't mind any of the "general bad parts" at all. Sure it might be nice if they used more ES6, but for the most part I don't expect it to have an effect on anything (maybe that's why they didn't do it - changing things just for the sake of changing them is asking for bugs). I don't care for TypeScript either, and the giant files haven't really bothered me with MV because I can just search for what I need.

(now getting window colors doesn't have to do weird hacks like new Window_Base().normalColor() and the codebase makes it clear that windows aren't the only ones that are supposed to use window colors)
Slightly off-topic, but wouldn't it make more sense to do Window_Base.prototype.normalColor() rather than new Window_Base().normalColor()? It's only slightly less weird and is even longer to type, admittedly, but it doesn't create an object only to look up a static value either.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
Almost forgot this general bad part - Insufficient documentations on many supposedly private functions/method.
I know that those codes are supposed to be edited by those already having at least a basic knowledge on what the involved problem and solution domain do in general, but many plugin developers don't even have that in many aspects of the default RMMZ codebase, causing them to lack the means available and needed for them to extend their plugin development comfort zones effectively and efficiently.
For instance, can you form a solid understanding on how the following instance method works on its own in details within minutes?
JavaScript:
Tilemap.prototype._addAutotile = function(layer, tileId, dx, dy) {
    const kind = Tilemap.getAutotileKind(tileId);
    const shape = Tilemap.getAutotileShape(tileId);
    const tx = kind % 8;
    const ty = Math.floor(kind / 8);
    let setNumber = 0;
    let bx = 0;
    let by = 0;
    let autotileTable = Tilemap.FLOOR_AUTOTILE_TABLE;
    let isTable = false;

    if (Tilemap.isTileA1(tileId)) {
        const waterSurfaceIndex = [0, 1, 2, 1][this.animationFrame % 4];
        setNumber = 0;
        if (kind === 0) {
            bx = waterSurfaceIndex * 2;
            by = 0;
        } else if (kind === 1) {
            bx = waterSurfaceIndex * 2;
            by = 3;
        } else if (kind === 2) {
            bx = 6;
            by = 0;
        } else if (kind === 3) {
            bx = 6;
            by = 3;
        } else {
            bx = Math.floor(tx / 4) * 8;
            by = ty * 6 + (Math.floor(tx / 2) % 2) * 3;
            if (kind % 2 === 0) {
                bx += waterSurfaceIndex * 2;
            } else {
                bx += 6;
                autotileTable = Tilemap.WATERFALL_AUTOTILE_TABLE;
                by += this.animationFrame % 3;
            }
        }
    } else if (Tilemap.isTileA2(tileId)) {
        setNumber = 1;
        bx = tx * 2;
        by = (ty - 2) * 3;
        isTable = this._isTableTile(tileId);
    } else if (Tilemap.isTileA3(tileId)) {
        setNumber = 2;
        bx = tx * 2;
        by = (ty - 6) * 2;
        autotileTable = Tilemap.WALL_AUTOTILE_TABLE;
    } else if (Tilemap.isTileA4(tileId)) {
        setNumber = 3;
        bx = tx * 2;
        by = Math.floor((ty - 10) * 2.5 + (ty % 2 === 1 ? 0.5 : 0));
        if (ty % 2 === 1) {
            autotileTable = Tilemap.WALL_AUTOTILE_TABLE;
        }
    }

    const table = autotileTable[shape];
    const w1 = this._tileWidth / 2;
    const h1 = this._tileHeight / 2;
    for (let i = 0; i < 4; i++) {
        const qsx = table[i][0];
        const qsy = table[i][1];
        const sx1 = (bx * 2 + qsx) * w1;
        const sy1 = (by * 2 + qsy) * h1;
        const dx1 = dx + (i % 2) * w1;
        const dy1 = dy + Math.floor(i / 2) * h1;
        if (isTable && (qsy === 1 || qsy === 5)) {
            const qsx2 = qsy === 1 ? (4 - qsx) % 4 : qsx;
            const qsy2 = 3;
            const sx2 = (bx * 2 + qsx2) * w1;
            const sy2 = (by * 2 + qsy2) * h1;
            layer.addRect(setNumber, sx2, sy2, dx1, dy1, w1, h1);
            layer.addRect(setNumber, sx1, sy1, dx1, dy1 + h1 / 2, w1, h1 / 2);
        } else {
            layer.addRect(setNumber, sx1, sy1, dx1, dy1, w1, h1);
        }
    }
};
I'm sure that some of you are so familiar with adding autotiles that you can even reason about it(perhaps by dry running the codes alone while keeping everything inside your heads) within seconds, but I bet that many plugin developers won't have a clue on what these codes are trying to achieve(other than adding autotile of course), and sometimes more importantly, why this particular implementation's used.
Ideally, Tilemap.prototype._addAutotile should be broken down into several smaller pieces so we can reason about abstractions instead of concretions, but if that's not feasible in this case(I've tried to break this down into smaller pieces but failed as I've no idea on what these codes are about), even just several lines of comments can help a lot already.
Of course, it's not the only example I've found so far, but in my opinion, it's perhaps the biggest offender I've seen in the default RMMZ codebase(and I hope that there won't be even bigger offenders waiting to be found by me) :)
 
Last edited:

ThinkN

Villager
Member
Joined
Aug 11, 2020
Messages
11
Reaction score
16
First Language
English
Primarily Uses
RMMV
@DoubleX Thanks for sharing your analysis. This thread is a great read.

I definitely don't have the skills to get to know this codebase without proper documentation.

I decided to look at some of the external dependencies (open-source JS libraries) that changed between MV to MZ.

I'm impressed by the switch from lz-string to deflate compression. I copied the Pako library into an MV project to replace lz-string, and it resulted in 25% smaller saved game files. It also seemed to reduce the duration of saving by a few milliseconds.

Currently in MV, when you save your progress, the game becomes entirely frozen until saving completes. Because of this, auto-save plugins can make opening/closing the menu feel clunky. For MZ, I assume the switch to using the Promises for saving/loading (and the LocalForage library which has an async API for browser storage) was to minimize this jank, and allow the built-in auto-save functionality to work smoothly. But I can't really test this yet without the actual software.
 

??????

Diabolical Codemaster
Veteran
Joined
May 11, 2012
Messages
6,455
Reaction score
2,994
First Language
Binary
Primarily Uses
RMMZ
I spotted this one the other day:

Such a dumb mistake as well...

I've been busy writing my own code, and havent really looked into the default class stuff too much yet. Although I have been pretty happy with most of the changes I've seen. a lot of it is a great improvement over mv.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
There are some places where I'm not sure if they're bad parts, but to me they're still very questionable codes.

For instance:
JavaScript:
/**
 * Changes the size of the game screen.
 *
 * @param {number} width - The width of the game screen.
 * @param {number} height - The height of the game screen.
 */
Graphics.resize = function(width, height) {
    this._width = width;
    this._height = height;
    this._updateAllElements();
};

/**
 * The width of the game screen.
 *
 * @type number
 * @name Graphics.width
 */
Object.defineProperty(Graphics, "width", {
    get: function() {
        return this._width;
    },
    set: function(value) {
        if (this._width !== value) {
            this._width = value;
            this._updateAllElements();
        }
    },
    configurable: true
});

/**
 * The height of the game screen.
 *
 * @type number
 * @name Graphics.height
 */
Object.defineProperty(Graphics, "height", {
    get: function() {
        return this._height;
    },
    set: function(value) {
        if (this._height !== value) {
            this._height = value;
            this._updateAllElements();
        }
    },
    configurable: true
});
Why does Graphics.resize always call Graphics._updateAllElements, even when the width and height don't change, while Graphics.width and Graphics.height only call Graphics._updateAllElements when the width and height changed respectively?
I guess that Graphics.resize should actually check for width height changes first before calling Graphics._updateAllElements, but I'm not sure on this one, it's just that it seems to me that Graphics.resize is inconsistent with Graphics.width and Graphics.height.

Similarly:
JavaScript:
/**
 * The width of the sprite without the scale.
 *
 * @type number
 * @name Sprite#width
 */
Object.defineProperty(Sprite.prototype, "width", {
    get: function() {
        return this._frame.width;
    },
    set: function(value) {
        this._frame.width = value;
        this._refresh();
    },
    configurable: true
});

/**
 * The height of the sprite without the scale.
 *
 * @type number
 * @name Sprite#height
 */
Object.defineProperty(Sprite.prototype, "height", {
    get: function() {
        return this._frame.height;
    },
    set: function(value) {
        this._frame.height = value;
        this._refresh();
    },
    configurable: true
});
Is calling Sprite.prototype._refresh really necessary when the width/height doesn't change?

And in the case of Windows:
JavaScript:
/**
 * The width of the window in pixels.
 *
 * @type number
 * @name Window#width
 */
Object.defineProperty(Window.prototype, "width", {
    get: function() {
        return this._width;
    },
    set: function(value) {
        this._width = value;
        this._refreshAllParts();
    },
    configurable: true
});

/**
 * The height of the window in pixels.
 *
 * @type number
 * @name Window#height
 */
Object.defineProperty(Window.prototype, "height", {
    get: function() {
        return this._height;
    },
    set: function(value) {
        this._height = value;
        this._refreshAllParts();
    },
    configurable: true
});

/**
 * The size of the padding between the frame and contents.
 *
 * @type number
 * @name Window#padding
 */
Object.defineProperty(Window.prototype, "padding", {
    get: function() {
        return this._padding;
    },
    set: function(value) {
        this._padding = value;
        this._refreshAllParts();
    },
    configurable: true
});

/**
 * The size of the margin for the window background.
 *
 * @type number
 * @name Window#margin
 */
Object.defineProperty(Window.prototype, "margin", {
    get: function() {
        return this._margin;
    },
    set: function(value) {
        this._margin = value;
        this._refreshAllParts();
    },
    configurable: true
});
Is calling Window.prototype._refreshAllParts really necessary when the width/height/padding/margin doesn't change?

Also, another general bad parts: The existence of many magic literals, like magic numbers and strings(not all of them are as obvious as 1000 for milliseconds or 1024 for B/KB/MB/etc), and there are simply too many to be listed here.
I simply wish that at least some of those magic literals to be replaced with well-named constants to increase code readability :)
 

??????

Diabolical Codemaster
Veteran
Joined
May 11, 2012
Messages
6,455
Reaction score
2,994
First Language
Binary
Primarily Uses
RMMZ
I'm sure as the days go on we will notice more and more things like that. Its RM after all :D
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
As I've just finished rewriting the manager parts of the codebase into the ES6 standard with some code quality improvements, I've found some more good and bad parts there that are more noticeable than those not mentioned here:
Good parts -
1. The use of zips from pako
2. The use of local storages from localForage
3. The ability to access node modules like fs and path(these are for direct filesystem accesses in effective and efficient manners), implying that MZ does indeed have native npm supports
4. The proper use of Promises to manage asynchronous operations even better
Bad parts -
Quite some intentional exception swallowing, as if those exceptions are used as control flows(there's not even a log in those really empty catches), and I'm not sure whether if/else really can't be used in any of those cases :)
 

Kino

EIS Game Dev
Veteran
Joined
Nov 27, 2015
Messages
551
Reaction score
787
First Language
English
Primarily Uses
RMMV
I see issues as I'm rewriting the code in another language. There are a few issues in the Color Manager that mirror MV. Will make optimization edits later.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
As I'm reading some parts of the rmmz_objects.js, I've realized a significant bad part and 1 questionable part:

Bad part -
RMMZ hardcodes the game loop FPS to be 60.
In RMMV, you can simply change SceneManager._deltaTime to have any game loop fps you want(but the rendering loop fps is of course another story), while in RMMZ, the game loop FPS is controlled by the PIXI.Ticker in PIXI.Application under Graphics(Graphics._app.ticker.deltaMs).
As the default deltaMs is 16.66, which is 60fps, and RMMZ doesn't even touch that part, it means it's assumed to be always 60fps.
What's more, at least in places like Game_System and Game_Timer, the fps directly hardcoded to be the constant 60(Game_System.prototype.playtime and Game_Timer.prototype.seconds).
To fix it, I'd suggest adding a fps accessor in Graphics, that will set the Graphics._app.ticker.deltaMs whenever that fps accessor's set with new values(then replace all places hardcoding the constant 60 as the fps with this new Graphics fps accessor).
It's useful for some plugins to perform performance stress test by increasing the game loop fps to 120(FHD do support 120Hz), and some RM users to cap the game loop fps to 30 to target users with less powerful machines.

Questionable part -
Like in RMMV, RMMZ assumes all numeric values in variables must be integers, causing variables supposedly to store numbers with decimals to have to store them in the string form(like "1.6" instead of 1.6) and convert them back to numeric values upon using(like +$gameVariables.value(1) instead of $gameVariables.value(1)).
If a RM user intentionally stores numbers with decimals into variables, we should assume that that user really knows what he/she's truly doing, instead of making this intentional operation more complicated and convoluted than it needs to be.
Therefore, I suggest that the Math.floor part in setValue should be simply removed.
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
I've finished reading Game_Action, and am quite surprised by the sheer number of nontrivial bad parts there, even though only one of them is really critical:
It directly hardcodes the formula Math.floor(this.item().tpGain * this.subject().tcr) into gainSilentTp, but there will be some plugins wanting to apply some custom tp gain formulae for some skills/items, so Game_Action.prototype.applyItemUserEffect should be rewritten into something like this.subject().gainSilentTp(this._newSilentTp());
It directly hardcodes the condition to add debuff as Math.random() < target.debuffRate(effect.dataId) * this.lukEffectRate(target), but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectAddDebuff should be rewritten into something like this:
JavaScript:
if (!this._isAddDebuff(target, effect)) return;
target.addDebuff(effect.dataId, effect.value1);
this.makeSuccess(target);
It directly hardcodes the condition to remove state as Math.random() < effect.value1, but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectRemoveState should be rewritten into something like this:
JavaScript:
if (!this._isRemoveState(target, effect)) return;
target.removeState(effect.dataId);
this.makeSuccess(target);
It directly hardcodes the condition to add normal state as Math.random() < effect.value1 for certain hits and Math.random() < effect.value1 * target.stateRate(effect.dataId) * this.lukEffectRate(target) for the rest, but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectAddNormalState should be rewritten into something like this:
JavaScript:
if (!this._isAddNormState(target, effect)) return;
target.addState(effect.dataId);
this.makeSuccess(target);
It directly hardcodes the condition to add attack state as Math.random() < effect.value1 * target.stateRate(stateId) * this.subject().attackStatesRate(stateId) * this.lukEffectRate(target), but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectAddAttackState should be rewritten into something like this:
JavaScript:
this.subject().attackStates().forEach(stateId => {
    if (!this._isAddAtkState(target, effect)) return;
    target.addState(stateId);
    this.makeSuccess(target);
});
On a side note: The condition on adding attack state itself's questionable - What if the attack skill's certain hit? Why the state rate, attack state rate and luk effect rate are still considered with certain hit attacks, when they're ignored in other certain hit skills? It's as if the assumption that the attack skill can't be certain hit is hardcoded.
It directly hardcodes the tp gain as Math.floor(effect.value1), but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectGainTp should be rewritten into something like this:
JavaScript:
const value = this._gainedTp(target, effect);
if (value === 0) return;
target.gainTp(value);
this.makeSuccess(target);
It directly hardcodes the mp recovery as Math.floor((target.mmp * effect.value1 + effect.value2) * target.rec * this.subject().pha) for items and Math.floor((target.mmp * effect.value1 + effect.value2) * target.rec) for the rest, but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectRecoverMp should be rewritten into something like this:
JavaScript:
const value = this._recoveredMp(target, effect);
if (value === 0) return;
target.gainMp(value);
this.makeSuccess(target);
It directly hardcodes the hp recovery as Math.floor((target.mhp * effect.value1 + effect.value2) * target.rec * this.subject().pha) for items and Math.floor((target.mhp * effect.value1 + effect.value2) * target.rec) for the rest, but there will be some plugins wanting to make some special cases to use some other formulae for some skills/items, so Game_Action.prototype.itemEffectRecoverHp should be rewritten into something like this:
JavaScript:
const value = this._recoveredHp(target, effect);
if (value === 0) return;
target.gainHp(value);
this.makeSuccess(target);
It swallows errors in the damage formula(those not returning numbers and those resulting in game crashes) by forcing the damage to be 0 without letting RM users know that, so users have to figure out by themselves whether a skill/item making 0 damage is due to damage formula errors or really legitimate damage.
Of course, hardcoding any error reporting mechanisms into damage formula errors would risk letting the RM players know what they shouldn't know if they managed to open the console(well, in this case, users having basic knowledge on what the default RMMZ implementation does in general will be able to do so even without these error reporting mechanisms anyway), but Game_Action.prototype.evalDamageFormula should at least let plugins add those error reporting mechanisms under certain conditions specified by plugin users(like when a game switch is on), so it should be rewritten into something like this:
JavaScript:
try {
    const item = this.item();
    const a = this.subject(); // eslint-disable-line no-unused-vars
    const b = target; // eslint-disable-line no-unused-vars
    const v = $gameVariables._data; // eslint-disable-line no-unused-vars
    const sign = [3, 4].includes(item.damage.type) ? -1 : 1;
    const value = Math.max(eval(item.damage.formula), 0);
    if (!isNaN(value)) return value * sign;
    throw new Error(`${item.damage.formula} doesn't return a number!`);
} catch (e) {
    this._onEvalDamageFormulaError(e);
    return 0;
}
Now, perhaps with the aid of plugins(which is now easy, simple and small tasks to implement instead of nearly impossible without creating bigger problems), RM users can effectively and efficiently control whether those damage formula errors should be shown, and enabling this can be useful during the testing phase, while disabling this can be useful right before publishing the game.
It makes plugins needlessly complicated and convoluted to add/edit/remove functionality when the action does hit the target, and when the action damage type is indeed damage to be applied(i.e., anything other than none), so Game_Action.prototype.apply should be rewritten into something like this:
JavaScript:
const result = target.result();
this.subject().clearResult();
result.clear();
result.used = this.testApply(target);
result.missed = this._isMissed(target);
result.evaded = this._isEvaded(target);
result.physical = this.isPhysical();
result.drain = this.isDrain();
if (result.isHit()) this._applyHit(target);
this.updateLastTarget(target);
And Game_Action.prototype._applyHit can be something like this:
JavaScript:
if (this.item().damage.type > 0) this._applyDamage(target);
for (const effect of this.item().effects) {
    this.applyItemEffect(target, effect);
}
this.applyItemUserEffect(target);
Actually, the original Game_Action.prototype.apply also makes plugins needlessly complicated and convoluted to change how whether the action's missed or evaded determined, so the above rewritten version also introduced Game_Action.prototype._isMissed and Game_Action.prototype._isEvaded.
It's basically the base speed, and with the action item speed bonus if the action has an item, and attack speed bonus if the action's attack, but the way it's written causes plugins to be harder to edit the base speed, which is default to this.subject().agi + Math.randomInt(Math.floor(5 + this.subject().agi / 4)), than it needs to be, so Game_Action.prototype.speed should be rewritten into something like this:
JavaScript:
let speed = this._baseSpeed();
if (this.item()) speed += this.item().speed;
if (this.isAttack()) speed += this.subject().attackSpeed();
return speed;
It makes plugins harder to edit the random target selection than it needs to be, as it immediately assigns the decided random target index into Game_Action.prototype._targetIndex(and calls Game_Action.prototype.clear if there's no such target), causing plugins trying to edit the random target selection to have an excessive amount of redundant works to do, so Game_Action.prototype.decideRandomTarget should be rewritten into something like this:
JavaScript:
const target = this._randomTarget();
target ? this._targetIndex = target.index() : this.clear();
Game_Action.prototype.evaluateWithTarget still calls Game_Action.prototype.makeDamageValue without even trying to sanitize the damage formula first, causing any auto battler actor using skills/items with damage formulae doing anything other than returning damage to cause those side effects to leak when those auto battle actors input actions.
Therefore, Game_Action.prototype.evaluateWithTarget should be rewritten into something like this:
JavaScript:
if (!this.isHpEffect()) return;
const value = this._evalDamageWithoutCri(target);
if (this.isForOpponent()) return value / Math.max(target.hp, 1);
return Math.min(-value, target.mhp - target.hp) / target.mhp;
Game_Action.prototype._evalDamageWithoutCri can be something like this:
JavaScript:
let baseValue = 0;
try {
    const item = this.item();
    const [a, b, v] = [this.subject(), target, $gameVariables._data];
    const sign = [3, 4].includes(item.damage.type) ? -1 : 1;
    const regex = new RegExp(".*[};] *", "gim");
    const formula =  this.item().damage.formula.replace(regex, "");
    const value = eval(formula);
    if (!isNaN(value)) baseValue = Math.max(value, 0) * sign;
    throw new Error(`${formula} doesn't return a number!`);
} catch (e) {
    this._onEvalDamageFormulaErr(e);
}
let value = baseValue * this.calcElementRate(target);
if (this.isPhysical()) value *= target.pdr;
if (this.isMagical()) value *= target.mdr;
if (baseValue < 0) value *= target.rec;
value = this.applyVariance(value, this.item().damage.variance);
return Math.round(this.applyGuard(value, target));

To be fair, Game_Action also has some improvements compared to that in the default RMMV codebase, like:
1. Breaking Game_Action.prototype.targetsForOpponents down into Game_Action.prototype.randomTargets and Game_Action.prototype.targetsForAlive
2. Breaking Game_Action.prototype.targetsForFriends down into Game_Action.prototype.targetsForDead, Game_Action.prototype.targetsForAlive and Game_Action.prototype.targetsForDeadAndAlive

I've also finished reading Game_BattlerBase, and so far I've only found 1 bad part inside that class: The lack of the event listener when a battler becomes unrestricted(i.e., the counterpart of onRestrict).
Fortunately, fixing it is as easy as rewriting Game_BattlerBase.prototype.addNewState into something like this:
JavaScript:
if (stateId === this.deathStateId()) this.die();
const wasRestricted = this.isRestricted();
this._states.push(stateId);
this.sortStates();
const isRestricted = this.isRestricted();
if (!wasRestricted && isRestricted) this.onRestrict();
if (wasRestricted && !isRestricted) this._onUnrestrict();
However, the very existence of Game_BattlerBase indicates a very serious architectural flaw: The battler object's simply too big due to having too many responsibilities.
First, the reason of Game_BattlerBase being very smelly is that, it's an abstract class but with only 1 child, and that child, Game_Battler, is itself an abstract class.
Normally, inheritance's supposed to model the "is a" relationship, like "Game_Actor is a Game_Battler" and "Game_Enemy is a Game_Battler", and these relationships are totally fine.
On the other hand, "Game_Battler is a Game_BattlerBase" clearly just doesn't make any sense, and the only reason for this anti-pattern to exist is that Game_BattlerBase's trying to extract some responsibilities from Game_Battler(otherwise Game_Battler would be a god object), but in a bad way.

A good way is to favor composition over inheritance in this case, but doing so would need to know the responsibilities of the battler object, which are basically the following:
1. Integrates all data used by the battler
2. Manipulates all battler states
3. Causes side effects to the externals
Therefore, the whole Game_BattlerBase should've never existed, and the battler object should have the following helpers:
1. this._dao - The battler Data Access Object of class Game_BattlerData
2. this._stateSet - The battler state set container of class Game_BattlerStateSet having tons of Value Objects
3. this._sensors - The helper that changes this._stateSet based on the side effects applied to the battler from the externals(of course, this._sensors also have access to this._dao)
4. this._effectors - The helper that causes side effects to the externals based on this._stateSet, this._dao and the commands issued to the battler

While this architecture is still far from perfect, it should at least make the responsibilities currently cluttered in the battler object more decomposed and organized:
Proposed RMMZ Game_Battler Design.png
The simplified battler object workflow will be as follows:
1. When the battler's just created, the battler initializes Game_BattlerStateSet, which uses the data returned by Game_BattlerData.
2. When the battler receives a command that takes side effects from the externals, the battler delegates that command with its arguments to Game_BattlerSensors, which combines the current state set from Game_BattlerStateSet, data returned by Game_BattlerData and the command arguments into the new state set written to Game_BattlerStateSet.
3. When the battler receives a command that makes side effects to the externals, the battler delegates that command with its arguments to Game_BattlerEffectors, which combines the current state set from Game_BattlerStateSet, data returned by Game_BattlerData and the command arguments to make side effects to the externals.

In the case of actors and enemies:
1. Game_ActorData and Game_EnemyData will be the children of Game_BattlerData
2. Game_ActorStateSet and Game_EnemyStateSet will be the children of Game_BattlerStateSet
3. Game_ActorSensors and Game_EnemySensors will be the children of Game_BattlerSensors
4. Game_ActorEffectors and Game_EnemyEffectors will be the children of Game_BattlerEffectors
Of course, this can lead to class bloat issues when there are many battler types other than actors and enemies, but it's unlikely enough to be ignored in the case of RMMZ until it does materialize.

As a bonus, if implemented well, this structure will also lead to better battler testability, as the Game_BattlerData can be mocked with test data, Game_BattlerStateSet can be mocked with predefined state sets, Game_BattlerSensors can be fed by fake command arguments, and most importantly, Game_BattlerEffectors can be stubbed into dummies that won't actually create side effects to the externals, or only those that are tightly controlled by the test fixtures that can be easily cleaned up.

Unfortunately, the biggest trouble of making this change is that, any meaningful implementation will be breaking changes, meaning that it's already too late for the default RMMZ codebase to use this battler object design instead.
While the current Game_Battler with Game_BattlerBase does work, this setup's one of the reason to be complicated and convoluted to turn automated battler tests into easy, simple and small tasks, and improved battler testability will definitely benefit many plugin developers writing battle-related plugins.
 
Last edited:

??????

Diabolical Codemaster
Veteran
Joined
May 11, 2012
Messages
6,455
Reaction score
2,994
First Language
Binary
Primarily Uses
RMMZ
Huge list of bad things. Two good things. Pretty much sums it up :D
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
I've just finished reading Game_Battler, and while it has even more bad parts than Game_Action(but none in Game_Battler is critical unlike there's 1 in Game_Action), I think it's actually understandable, as many of them come from the TPBS, and for the sake of being even more forgiving to those bad parts, I'd assume that the RM devs have no prior experience on writing ATB systems(on the other hand, if they're already experienced on this before implementing TPBS, I'd say that they're doing a MASSIVE DISSERVICE to the plugin developers, especially for those being very familiar with implementing ATB system plugins).
For this reason, the following list's far from being exhaustive, as I consider those not mentioned there trivial compared to those in this list:
The game battler's actually storing some states for the battler sprite, like setActionState, performMiss, performActionStart, isChanting, startDamagePopup, weaponImageId, isWeaponAnimationRequested, requestMotion and clearEffect.
Such methods should actually belong to Sprite_Battler, as they're called as the battle log window(Window_BattleLog), which has direct accesses to the battler sprite set(Spriteset_Battle), which has direct accesses to the battler sprites, meaning that the entity communications are more complicated and convoluted than what has to be:
1. The battle log window requests the battler object to store some new states on how the battler sprite should behave
2. The battler sprite queries the battler object for the battler sprite states stored in that battler object
3. The battler sprite acts according to those queried states
Whereas if the battle log window directly requests the battler sprites to store new states via the battler sprite set, the battler sprite then just need to act according to its internal states, meaning that now the battler sprite will only requests battler data from the battler object, and the battler object doesn't need to store any battler sprite state and thus doesn't need to know any battler sprite business logic, causing the code quality of both Game_Battler and Sprite_Battler to improve drastically, due to the responsibilities being decomposed and distributed even more properly.
Specifically, at least these methods should be added:
JavaScript:
Game_Battler.prototype.isTpbActing = function() { return this._tpbState === "acting"; }
Game_Battler.prototype.isTpbCharging = function() { return this._tpbState === "charging"; }
Game_Battler.prototype.isTpbCasting = function() { return this._tpbState === "casting"; }
Game_Battler.prototype.onTpbReady = function() { this._tpbState = "ready"; }
Game_Battler.prototype.isEndTpbCharging = function() { return this._tpbChargeTime >= 1; }
Game_Battler.prototype.isEndTpbCasting = function() { this._tpbCastTime >= this.tpbRequiredCastTime(); }
Game_Battler.prototype.isTpbIdle = function() { return !this.canMove() || this.isTpbCharged(); }
For instance, there can be some plugins letting their users set some casting skills to add special states on the battler casting those skills right after the casting start, and those states will only be removed when the casting ends.
With Game_Battler.prototype.onTpbReady, implementing such features will become easy, simple and small tasks, whereas its absence will cause plugins to have to struggle on Game_Battler.prototype.updateTpbCastTime.

Game_Battler.prototype.isEndTpbCharging is useful for some plugins to let their users to control the AI behaviors of enemies and autobattle/confused actors by adding a delay that can be changed at anytime, so those battlers don't always have to input actions right after they become able to do so.
For instance, Game_Battler.prototype.isEndTpbCharging can be changed into something like this:
JavaScript:
Game_Battler.prototype.isEndTpbCharging = function() { return this._tpbChargeTime >= 1 + this._tpbChargeDelay(); }
So the delay essentially serves as a watchdog timer, meaning that plugin users can, under some conditions, keep that delay from reaching 0(of course Game_Battler.prototype._tpbChargeTime still needs to be capped to 1), but when it's the advantageous moments for those AI to actually input the actions, the delay can be set to 0 so those battlers will then input actions immediately.

Similarly, Game_Battler.prototype.isEndTpbCasting is useful for some plugins to let their users to set some skills/items to be able to be overchanged beyond the ordinary maximum, causing the skills/items to be executed to become even more powerful, or set some skills/items to be able to be executed before they're fully charged, causing the skills/items to be executed to be less powerful but also costing less time to cast and/or the battlers casting the skills/items to receive some extra penalties.
This can be done by binding a hotkey for each actor, and when the key's pressed, the casting can be overcharged, while when the key's released, the casting will be finished, regardless of whether it's not less fully charged, just fully charge or overcharged.
In the case of enemies and autobattle/confusion battlers, a script call for that key pressing event and another script call for that key releasing event can do the same trick.
All these can be done by overriding Game_Battler.prototype.isEndTpbCasting, even though quite some effort and work will be involved, but things will become much, much more complicated and convoluted without that method to deal with.
While currently the battler TPBS state(like the current battler TPBS phase, the charging value, casting value, idle time, etc) set is easy, simple and small, it'll become larger and larger when more and more plugins add more and more features to the TPBS.
Without a class(like Game_BattlerTimeProgress), it effectively encourages and recommends those plugins to just throw everything into Game_Battler and Game_BattlerBase, quickly turning it into a smelly kitchen sink that almost no one will bother or even can clean up later anymore, especially when the battler object's already risking being a god object even without the TPBS.
It's because many of those plugins are easy, simple and small themselves, and it makes no sense for them to make a new class just for their tiny changes, and doing so can actually lead to tons of tiny classes, which can be a worse anti-pattern.
While kitchen sink classes aren't always inherently bad, it's very hard to be kept under control in general, and it'll only make testing and debugging the battler object harder and harder(let alone writing a comprehensive automated test suite), which can be very troublesome for plugins editing the battle action input and/or execution flows, because those flows are already costly to test to begin with.
Unfortunately, again any meaningful implementation to extract the TPBS business logic from the Game_Battler into a new class will be a breaking change, meaning that it's also too late for RMMZ to make this move at this point.
Simply put, almost everything in TPBS is recalculated per frame per battler, rather than being cached.
For instance, Game_Battler.prototype.updateTpbCastTime calls this.tpbRequiredCastTime() per frame:
JavaScript:
Game_Battler.prototype.updateTpbCastTime = function() {
    if (this._tpbState === "casting") {
        this._tpbCastTime += this.tpbAcceleration();
        if (this._tpbCastTime >= this.tpbRequiredCastTime()) {
            this._tpbCastTime = this.tpbRequiredCastTime();
            this._tpbState = "ready";
        }
    }
};
And Game_Battler.prototype.tpbRequiredCastTime doesn't cache anything:
JavaScript:
Game_Battler.prototype.tpbRequiredCastTime = function() {
    const actions = this._actions.filter(action => action.isValid());
    const items = actions.map(action => action.item());
    const delay = items.reduce((r, item) => r + Math.max(0, -item.speed), 0);
    return Math.sqrt(delay) / this.tpbSpeed();
};
Where Game_Battler.prototype.tpbSpeed is this:
JavaScript:
Game_Battler.prototype.tpbSpeed = function() {
    return Math.sqrt(this.agi) + 1;
};
Imagine a battle with 4 actors and 8 enemies(the default maximum on each side), and the average number of action slots per battler is 5.
This leads to (4 + 8) * 5 = 60 queries on Game_Action.prototype.isValid and Game_Action.prototype.item per frame, and both Math.max and Math.sqrt aren't free, even though they still costs only a little per call.
Bear in mind that it's done per frame, which is about 16ms, and that 16ms has many other things to do.
So while these unoptimized codes seem to be trivial on their own, the combined performance penalty from many such tiny ineffectiveness and inefficiencies should never be underestimated, especially when a project has many plugins and is targeting users with low-end mobiles.

If there are plugins trying to edit the TPB cast time, but those edits are based on the delay variable, those plugins will have no choice but to either abandon caching altogether, which can lead to significant performance issues if the plugin users use tons of notetags, or be forced to cache the delay variable as well, which is effectively doing what should've been done by the default RMMZ codebase implemenetations.
It's because if the value of the delay variable changes, that change can propagate to all subsequent changes introduced by such plugins, so for the plugins to cache those new changes introduced by themselves, they first need to ensure the delay variable hasn't changed, which will obviously involve caching.

To fix it, some sort of easy, simple and small caches can be used, by making use of the fact that all these calculations are deterministic and their inputs won't change without reasons, meaning that the cache of these values only need to be invalidated when at least 1 of the reasons to change the cached value might have triggered.
For instance, changing the battler agi in any way should trigger Game_Battler.prototype.refresh, which isn't a hotspot(at least far from being called per frame), so it's a good place to invalidate the cache(even though calling Game_Battler.prototype.refresh doesn't always mean the battler agi has changed).
The same applies to makeActions, clearActions, removeCurrentAction, forceAction, where the action slot lists will be changed there, and thus causing the cahce to have to be invalidated.
On a side note: An action validity shouldn't be changed without calling Game_Battler.prototype.refresh, and changing the action item during casting will itself create even greater problems unless those changes are done very consciously, and if they're so conscious, calling Game_Battler.prototype.refresh as well shouldn't be too demanding in those cases.
In short, if the fastest enemy is the only one being unmovable(paralyzed, slept, etc), the battle turn count can be increased faster than that of the individual turn count of the fastest enemy that's still movable, causing the disconnect between the battle turn(and thus battle events) and the remaining movable enemies to be larger and larger, especially when that fastest enemy(now being unmovable) is much, much faster than the other enemies(and the surprise will only be larger if that enemy's much, much faster than the faster actor as well), all until that fastest enemy becomes movable again.
Proof:
JavaScript:
Game_Battler.prototype.updateTpb = function() {
    if (this.canMove()) {
        this.updateTpbChargeTime();
        this.updateTpbCastTime();
        this.updateTpbAutoBattle();
    }
    if (this.isAlive()) {
        this.updateTpbIdleTime();
    }
};
Game_Battler.prototype.updateTpbIdleTime = function() {
    if (!this.canMove() || this.isTpbCharged()) {
        this._tpbIdleTime += this.tpbAcceleration();
    }
};
Game_Battler.prototype.isTpbTimeout = function() {
    return this._tpbIdleTime >= 1;
};
BattleManager.updateTpbBattler = function(battler) {
    if (battler.isTpbTurnEnd()) {
        battler.onTurnEnd();
        battler.startTpbTurn();
        this.displayBattlerStatus(battler, false);
    } else if (battler.isTpbReady()) {
        battler.startTpbAction();
        this._actionBattlers.push(battler);
    } else if (battler.isTpbTimeout()) {
        battler.onTpbTimeout();
        this.displayBattlerStatus(battler, true);
    }
};
Game_Battler.prototype.onTpbTimeout = function() {
    this.onAllActionsEnd();
    this._tpbTurnEnd = true;
    this._tpbIdleTime = 0;
};
Game_Battler.prototype.isTpbTurnEnd = function() {
    return this._tpbTurnEnd;
};
Game_Battler.prototype.startTpbTurn = function() {
    this._tpbTurnEnd = false;
    this._tpbTurnCount++;
    this._tpbIdleTime = 0;
    if (this.numActions() === 0) {
        this.makeTpbActions();
    }
};
Game_Battler.prototype.turnCount = function() {
    if (BattleManager.isTpb()) {
        return this._tpbTurnCount;
    } else {
        return $gameTroop.turnCount() + 1;
    }
};
Game_Troop.prototype.isTpbTurnEnd = function() {
    const members = this.members();
    const turnMax = Math.max(...members.map(member => member.turnCount()));
    return turnMax > this._turnCount;
};
BattleManager.checkTpbTurnEnd = function() {
    if ($gameTroop.isTpbTurnEnd()) {
        this.endTurn();
    }
};
BattleManager.endTurn = function() {
    this._phase = "turnEnd";
    this._preemptive = false;
    this._surprise = false;
    if (!this.isTpb()) {
        this.endAllBattlersTurn();
    }
};
BattleManager.updatePhase = function(timeActive) {
    switch (this._phase) {
        case "start":
            this.updateStart();
            break;
        case "turn":
            this.updateTurn(timeActive);
            break;
        case "action":
            this.updateAction();
            break;
        case "turnEnd":
            this.updateTurnEnd();
            break;
        case "battleEnd":
            this.updateBattleEnd();
            break;
    }
};
BattleManager.updateTurnEnd = function() {
    if (this.isTpb()) {
        this.startTurn();
    } else {
        this.startInput();
    }
};
BattleManager.startTurn = function() {
    this._phase = "turn";
    $gameTroop.increaseTurn();
    $gameParty.requestMotionRefresh();
    if (!this.isTpb()) {
        this.makeActionOrders();
        this._logWindow.startTurn();
        this._inputting = false;
    }
};
Game_Troop.prototype.increaseTurn = function() {
    const pages = this.troop().pages;
    for (let i = 0; i < pages.length; i++) {
        const page = pages[i];
        if (page.span === 1) {
            this._eventFlags[i] = false;
        }
    }
    this._turnCount++;
};
Actually, the surprise comes from the conceptual level rather than the implementation level, so if it turns out that the relationship between the individual turn and battle turn are too confusing to some RM users and players, the only way to fix it is to make some plugins to change the very definitions of the battle turn, to something like a fixed number of ATB frame updates elapsed or the number of actions executed. But then the battle system won't be TPBS anymore, as it'd become a standard ATB system instead.
However, there can still be some workarounds to this, like by rewriting Game_Battler.prototype.updateTpbIdleTime into something like the following, even though it won't fix the root cause of the issue:
JavaScript:
Game_Battler.prototype.updateTpbIdleTime = function() {
    if (this.isTpbIdle()) this._onUpdateTpbIdleTime();
};
If it turns out that the idle enemy's updating the battle turn too quickly(this can be checked by comparing that enemy turn count to the battle turn count), the Game_Battler.prototype._onUpdateTpbIdleTime can be edited to stop adding Game_Battler.prototype._tpbIdleTime altogether.
It hardcodes the starting tpb bar value to be this.tpbRelativeSpeed() * Math.random() * 0.5, while there will certainly be plugins wanting to let their users change that, so Game_Battler.prototype.initTpbChargeTime should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.initTpbChargeTime = function(advantageous) {
    const speed = this.tpbRelativeSpeed();
    this._tpbChargeTime = this._initializedTpbChargeTime(advantageous);
};
Game_Battler.prototype._initializedTpbChargeTime = function(advantageous) {
    if (this.isRestricted()) return 0;
    return advantageous ? 1 : this._initializedNormTpbChargeTime();
};
Game_Battler.prototype._initializedNormTpbChargeTime = function(advantageous) {
    return this.tpbRelativeSpeed() * Math.random() * 0.5;
};
Currently it's written this way:
JavaScript:
Game_Battler.prototype.addState = function(stateId) {
    if (this.isStateAddable(stateId)) {
        if (!this.isStateAffected(stateId)) {
            this.addNewState(stateId);
            this.refresh();
        }
        this.resetStateCounts(stateId);
        this._result.pushAddedState(stateId);
    }
};
If you want to add anything right before Game_Battler.prototype.addNewState or right after Game_Battler.prototype.refresh there, you'll have to check Game_Battler.prototype.isStateAddable and Game_Battler.prototype.isStateAffected again, and before calling the original Game_Battler.prototype.addState.
Needless to say, rewriting it into something like this will make the work for such plugins much easier, simpler and smaller tasks:
JavaScript:
Game_Battler.prototype.addState = function(stateId) {
    if (this.isStateAddable(stateId)) this._onAddState(stateId);
};
Game_Battler.prototype._onAddState = function(stateId) {
    if (!this.isStateAffected(stateId)) this._onAddNewState(stateId);
    this.resetStateCounts(stateId);
    this._result.pushAddedState(stateId);
};
Game_Battler.prototype._onAddNewState = function(stateId) {
    this.addNewState(stateId);
    this.refresh();
};
It hardcodes the assumption that the state removal due to damage must be by the chance fixed by the state, but there can be plugins letting users to set some states to have greater chance of removal when the damage's larger(like next to 0% chance to remove for almost 0 damage, and next to 100% chance to remove for damage being nearly 50% of mhp), so Game_Battler.prototype.removeStatesByDamage can be rewritten into something like this:
JavaScript:
Game_Battler.prototype.removeStatesByDamage = function() {
    this.states().forEach(this._removeStateByDamage, this);
};
Game_Battler.prototype._removeStateByDamage = function() {
    if (this._isRemoveStateByDamage(state)) this.removeState(state.id);
};
It hardcodes the initialized tp to be Math.randomInt(25), and some plugins will want to change that, so Game_Battler.prototype.initTp should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.initTp = function() { this.setTp(this._initializedTp()); }
It hardcodes the tp charged by damage tp to be Math.floor(50 * damageRate * this.tcr), and some plugins will want to change that, so Game_Battler.prototype.chargeTpByDamage should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.chargeTpByDamage= function() { this.gainSilentTp(this._chargedTpByDamage(damageRate)); }
It hardcodes the regenerated hp to be Math.max(Math.floor(this.mhp * this.hrg), -this.maxSlipDamage()), and some plugins will want to change that, so Game_Battler.prototype.regenerateHp should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.regenerateHp= function() {
    const value = this._regeneratedHp();
    if (value !== 0) this.gainHp(value);
};
It hardcodes the regenerated mp to be Math.floor(this.mmp * this.mrg), and some plugins will want to change that, so Game_Battler.prototype.regenerateMp should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.regenerateMp= function() {
    const value = this._regeneratedMp();
    if (value !== 0) this.gainHp(value);
};
It hardcodes the regenerated tp to be Math.floor(this.maxTp() * this.trg), and some plugins will want to change that, so Game_Battler.prototype.regenerateTp should be rewritten into something like this:
JavaScript:
Game_Battler.prototype.regenerateTp= function() { this.gainSilentTp(this._regeneratedTp()) };

In short, it seems to me that the RM devs didn't think of the possibilities opened by the TPBS innovations, or ATB systems in general, outside of their current implementations, so they didn't implement hooks and seams to make the implementations of those possibilities easier, simpler and smaller tasks. While it'd be asking too much for them to think of every such possibilities(especially if its turns out that they do have no prior experience on implementing ATB systems), they should at least have thought of some of them.
This also means that, without major refactoring, the current TPBS can only cater to easy, simple and small use cases, which are fortunately the majority. But for the minor complicated and convoluted use cases to be addressed by advanced complex plugins, those plugins are better off rewriting the whole TPBS from scratch, even though it'd risk raising tons of compatibility issues.
It's because the current implementation, while being an easy, simple and small task to have a solid understanding on how it works on its own in details and thus reason about it effectively and efficiently, is a bit limited even when plugins are taken into account, because some advanced ATB features, like the following:
1. Continuous action cost(actions costing a variable portion of the ATB bar rather than a fixed number of action slots with the full ATB bar)
2. Pooled ATB values(several battlers contributing to and using the same pooled ATB bar)
3. Multiple ATB bar types(like magic ATB bar for magical skills costing mp, special ATB bar for special skills costing tp, and normal ATB bar for attack, guard and items, meaning that skills costing both mp and tp will need both magical and special ATB bars to meet the action cost requirements)
4. The dreaded parallel actions(multiple actions can be executed by multiple battlers at the exact same frame, even though it's definitely asking too much here as it's perhaps the most demanding ATB system feature possible)
Demand a very, very well-designed ATB system architecture right from the start(otherwise the sheer amount of complexity introduced by those features would just be too hard to handle well), which is way, way beyond what's offered in the current default RMMZ codebase implementations(to be fair, coming up with such a well-designed ATB system architecture is itself insanely difficult even for veteran ATB system plugin developers).

Nevertheless, there's at least 1 good part in the TPBS implementation: The possibilities to use Action Times+ there:
JavaScript:
Game_Battler.prototype.startTpbTurn = function() {
    this._tpbTurnEnd = false;
    this._tpbTurnCount++;
    this._tpbIdleTime = 0;
    if (this.numActions() === 0) {
        this.makeTpbActions();
    }
};
Game_Battler.prototype.makeTpbActions = function() {
    this.makeActions();
    if (this.canInput()) {
        this.setActionState("undecided");
    } else {
        this.startTpbCasting();
        this.setActionState("waiting");
    }
};
Game_Battler.prototype.makeActions = function() {
    this.clearActions();
    if (this.canMove()) {
        const actionTimes = this.makeActionTimes();
        this._actions = [];
        for (let i = 0; i < actionTimes; i++) {
            this._actions.push(new Game_Action(this));
        }
    }
};
At the very least, the number of action slots in the TPBS is still dictated by the Action Times+, making plugins implementing different action costs for different skills/items(they demand different number of action slots per execution) easy, simple and small task(but still not to the point of being trivial though), like by removing the extra amount of action slots right after executing a skill/item demanding more than 1.
Of course, keeping Game_Battler.prototype.makeActions the same across both TPBS and turn based battle system will also make the codebase less complicated and convoluted, so at least the default RMMZ codebase has handled the Action Times+ in a good enough way.

P.S.: I wanted to include the Game_Actor parts here as well, but now I'm extremely tired...
 
Last edited:

Solar_Flare

Veteran
Veteran
Joined
Jun 6, 2020
Messages
523
Reaction score
230
First Language
English
Primarily Uses
RMMV
Game_Action.prototype.evaluateWithTarget still calls Game_Action.prototype.makeDamageValue without even trying to sanitize the damage formula first, causing any auto battler actor using skills/items with damage formulae doing anything other than returning damage to cause those side effects to leak when those auto battle actors input actions.
I honestly don't think it's worth jumping through hoops to support impure damage formulas. The right answer from a design standpoint would be to enforce the purity of the damage formula (ie, prevent side-effects altogether).

That said, I understand that a lot of people rely on the ability to add side-effects to the damage formula, so removing that misfeature would cause a major outcry. Still, I think your solution to the problem is overly complex. I believe the correct way to enforce purity is to make a copy of the battler object (probably using JsonEx.makeDeepCopy) and then discard it after you've evaluated the damage formula.

It's not perfect, mind you; if the formula changes global variables, then those changes would still happen. I don't know if there's a perfect solution; technically speaking I suppose you could clone the entire environment (more precisely, the "window" object) and somehow runt he damage formula in that temporary environment, but I don't know if there's actually a way to do that (plus I doubt it's very efficient).

Anyway, your code will always give the wrong answer if a formula has any if statements, so I think copying the object is not only simpler but also more correct. Your attempt at stripping off all impure parts of the formula doesn't even work - for example, the following formula would still produce its side-effects:

JavaScript:
b.isStateAffected(4) ? (a.addState(12), a.atk * 10 - b.def) : a.atk * 2 - b.def
On the other hand, by copying the object and evaluating the formula on the copy, even that formula's side-effects would be avoided. It's not perfect though - the following formula would produce its side-effects under both methods:

JavaScript:
b.isStateAffected(4) ? ($gameVariables.setValue(12, v[12] + 1), a.atk - b.def) : a.atk * (v[12] || 1) - b.def
This is probably solvable by making a copy of $gameVariables and reverting to that copy after the formula potentially alters it. Even that's not perfect (there are other globals the formula could change besides $gameVariables), but if you also do the same for $gameSwitches, it's probably good enough for nearly all complicated formulas.

And just for good measure, an example of a formula where your code does enforce purity but gives an incorrect result:

JavaScript:
if(b.isStateAffected(4)) a.atk * 4; else a.atk * 2 - b.def
Indeed, your code will cause an error here if I'm reading it correctly, because it strips everything up to the semicolon, which leaves just else a.atk * 2 - b.def which isn't valid JavaScript. Even if you fix this (eg by stripping out the "else" keyword too) it will give the wrong answer when the target is poisoned (or whatever state 4 is).

JavaScript:
    const regex = new RegExp(".*[};] *", "gim");
Side note... I find it very strange that you didn't write this like that?
JavaScript:
    const regex = /.*[};] */gim;
 

DoubleX

Just a nameless weakling
Veteran
Joined
Jan 2, 2014
Messages
1,708
Reaction score
856
First Language
Chinese
Primarily Uses
N/A
That said, I understand that a lot of people rely on the ability to add side-effects to the damage formula, so removing that misfeature would cause a major outcry. Still, I think your solution to the problem is overly complex. I believe the correct way to enforce purity is to make a copy of the battler object (probably using JsonEx.makeDeepCopy) and then discard it after you've evaluated the damage formula.
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).
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, 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.

As for my approach, I think I've to talk about it with more details:
1. The regular expression of sanitizing the damage formula upon evaluating actions to be inputted by auto battle actors is defaulted to /.*[};] */gim, but the actual implementation should allow script calls and/or plugin commands to change it to what suits the needs of the MZ users(so that regular expression might be a static Game_Action variable).
2. Fixing this bug demands MZ users to do some work on their sides - They must STANDARDIZE THEIR DAMAGE FORMULA so they can come up with an easy, simple and small regular expressions that will always remove all side effect parts and keep all damage returning part intact.
For instance, all damage formula should have structures like this(Separator is the part captued by the regular expression):
SideEffectPart Separator DamageReturningPart
Of course, sometimes this can lead to duplicate conditionals, and sometimes it's even just impossible, but I'd say that this has to be judged on a case-by-case basis, and the point of my proposed fix is to let MZ users have the option to make such judgments, and edit the regular expression until it works.

If, for some reasons, it's impossible to have a single regular expression to cover all damage formulae, the fix can even be enhanced to something like this:
JavaScript:
Game_Action.prototype.damageFormulaWithoutSideEffects = function(item) {
    const { id, damage: { formula } } = item;
    switch (id) {
        case 1: return formula.replace(regexA, "");
        case 2: return "damageFormulaWithoutSideEffects";
        case 3: return item.meta.damageFormulaWithoutSideEffects;
        default: return formula.replace(regexB, "");
    }
}
In short, it allows different damage formulae to be sanitized by different regular expressions, and it even allows users to return the whole hand-typed side effect free version without using regular expressions :)
 
Last edited:

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

Latest Threads

Latest Profile Posts

Hello I buyed RPG Maker VX Ace a long ago and I want to use it again but in the meantime I changed my pc but I lost the paper with my product key on so I can't use RPG maker again... someone can help me please ? or there is no hopes ?:'( ( I have a bad english I'm sorry )
I was away this days here in the forum but it was for a good cause!
The trailer of the demo of Luke Inn is on youtube and I am so happy!
I need to see the bugs of the game but I am happy that I have new content to show!

The last part of the modern interior will be ready soon. This will be a B-tile. With interior items for the kitchen, bathroom, toilet and storage room.
This frantic shooting ARPG plugin for MZ looks really great. Here is the link to a post from the official Japanese RPG Maker forum.

Forum statistics

Threads
102,941
Messages
996,122
Members
134,394
Latest member
tp0000
Top