fix memory bloat, and background preload.

[ghost]

Main strategy:

• empty loading changes to use reserveXXX
• reserveXXX reserves Bitmap. You can access that by loadXXX until reservation is cancelled.
• reserveXXX can cancel by reservation id.
• When reservation cancelled. That bitmap is ready to purge.

Purging Strategy:

• LFU (see ImageCache.js)

Bitmap is rewritten to state based. Original Bitmap Implementation is managed by flags. (see Bitmap.js)

Background Preload Strategy:

• Limits 1 connection (see RequestQueue.js)
• Clear queue when Scene changes.
• analyze event flow and background preload. (see Game_Interpreter.js)

Any question welcome!

[ivanpopelyshev]

If you do that, you also can remove my CacheMap and CacheEntry, they were made for that particular thing :)

[krmbn0576]

Found format-only changes! :-) Bitmap 317-320, 359, 726 ImageManager 89, 94, 101, 103, 110 (These may not be problematic because they are surrounded by changes) Scene_Base 1 Scene_Boot 1

[ghost]

Thanks a lot!

[krmbn0576]

Limits 1 connection (see RequestQueue.js)

Why? Parallel loading is evil?

Clear queue when Scene changes.

I think ImageManager.clearRequest(); is called only when Scene_Map terminated. Is this OK?

[ghost]

Why? Parallel loading is evil?

There may be another connections, so We'd better not consume connection limit for background loading. This loading request is lower priority.

I think ImageManager.clearRequest(); is called only when Scene_Map terminated. Is this OK?

Currently background loading occurs only Scene_Map, and canceling by another scene(such as Status) makes background loading useless.

[krmbn0576]

canceling by another scene(such as Status) makes background loading useless.

Well, canceling occurs no matter which the next scene is. "Useless" situation occurs anyway...

[ghost]

Well, canceling occurs no matter which the next scene is. "Useless" situation occurs anyway...

Oops you are right. I must consider about when to remove requests...

[ghost]

after finish review, I'll re-test them.

[krmbn0576]

BTW, I'm wondering if we should increase private methods any more. Plugin creators and scriptors may not know "_ is private", so it seems better not to define them except for rpg_core.js. In fact, the private methods of the rest five files are only Spriteset_Map#_canvasReAddParallax, SceneManager._getTimeInMs(WithoutMobileSafari) and the currently proposed SceneManager._doesNeedClearingRequest. None of these methods are from the beginning.

[krmbn0576]

Initially, the following implementation seems to be simpler.

Scene_Map.prototype.terminate = function() {
    // above and below omitted
    if (SceneManager.isNextScene(Scene_Map)) {
        ImageManager.clearRequest();
    }
};

[krmbn0576]

Currently background loading occurs only Scene_Map

Oh, I just now realized that this was wrong. Game_Interpreter is also used for BattleEvent!

[ghost]

I want to aggregate responsibility of clearing request.

Clearing request only occurs when scene is changed. and SceneManager is the only class that have responsibility of Scene-to-Scene management

As you say, there are some chance to need clearing request. (Scene_Battle)

[ghost]

Initially, the following implementation seems to be simpler.

after a bit, I also think the implementation is overpowered. I'll follow the suggestion.

[krmbn0576]

I still think that some ImageManager.loadXxx should be replaced by reserveXxx, because there are behaviors that expect to wait until it is loaded. (by using ImageManager.isReady)

Sprite_Picture#loadBitmap (first time only)
Spriteset_Map#loadTileset, _canvasReAddParallax, updateParallax (first time only)
Spriteset_Battle#battleback1,2Bitmap (first time only)
Sprite_Animation#loadBitmaps
Window_Message#updateLoading

Of course, it is rare case that these are purged before ImageManager.isReady is called, but it should be cared from semantics.

[ghost]

I still think that some ImageManager.loadXxx should be replaced by reserveXxx, because there are behaviors that expect to wait until it is loaded. (by using ImageManager.isReady)

reserveXXX fires loading and grantees to hold by ImageManager. Bitmap is still valid if it was purged from ImageManager, so We don't need replace them.

[krmbn0576]

Of course it's valid, but I am talking about the behavior of ImageManager.isReady. Previously, because all bitmaps were cached, ImageManager.isReady() === true indicated that "all bitmaps are ready." However, now it'll be true even if the purged bitmap is not ready. So the meaning of the method is broken.

[ghost]

Understood. The problem is, We break isReady semantics. I'll change isReady implementation.

[krmbn0576]

OK, finally I finished the review! We'll postpone the solution of reserveFace and request placeholder. After you re-test it and resolve the conflict, I'll approve it. LGTM! 🍣 😇 ❤️