Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replaceScene する度にメモリが増えてしまう。 #280

Open
luuthanhtuan opened this issue Aug 27, 2014 · 9 comments
Open

replaceScene する度にメモリが増えてしまう。 #280

luuthanhtuan opened this issue Aug 27, 2014 · 9 comments

Comments

@luuthanhtuan
Copy link

うちで作っているゲームはなぜメモリリークが発生しているのか調査したら、
今のreplaceScene 関数だと、replaceScene する度にメモリが増えてしまう。
新しいシーンをpushする前に、現在のシーンのすべてのchildNodeを取り去らなければならいようです。

replaceScene: function(scene) {            
    var currentScene = this.popScene();
    while (currentScene.childNodes.length > 0) {
       currentScene.removeChild(currentScene.childNodes[0]);
    }
    return this.pushScene(scene);
},
@oosamanet
Copy link

👍 自分の所でも困っていたので超助かりました

@jinnaiyuu
Copy link

私の所でも問題が解決しました。

NodeクラスがparentNodeを参照出来るので、childNodeのいずれかが参照可能だとSceneも参照可能になり、結局Scene以下のNodeが全て参照可能になってしまう、ということでしょう。

・Reference
Memory Management - JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Memory_Management

重要な改善だと思いますので、unit testも通りますし、pull requestを書いてはいかがでしょうか。

@rtsan
Copy link
Collaborator

rtsan commented Sep 9, 2014

SceneやSceneに追加したオブジェクトが削除されないのは、
Entityとその派生クラスが持つコレクションに参照が残ることが原因です。
5294236 の修正でも問題は解消しますが、置き換えられたSceneを再利用することができなくなってしまうので、
別の方法を取ったほうが良いと思います。
具体的には、以下のいずれかの形にしたいと思うのですが、いかがでしょうか。

  • enchant.Sceneに破棄メソッドを追加する
  • コレクション機能の有効/無効を切り替える方法を提供する
  • コレクション機能をクラスとして分離、使用者が任意に作成するようにする

@jinnaiyuu
Copy link

現在のAPIだとSceneの再利用の為にはスタック構造(pushScene, popScene)があるので、再利用しない場合のメソッド(自動で破棄してくれる)があると便利だと思います。スタックAPIがあるので、恐らく多くのユーザはreplaceSceneはSceneを自動破棄するメソッドだと想像すると思います。

なので、

  1. replaceSceneは再利用しない場合のメソッドとし、
  2. enchant.Sceneに破棄メソッドを追加し、ユーザ定義型のシーン切り替えもサポート
    とするのが良いのではないでしょうか。

こうすることでenchant.jsの強みである分かり易さを残しつつ機能を拡張出来ると思いますが、いかがでしょう。

コレクション機能の切り替えやクラスも今後欲しい所ですが、まずは破棄メソッドの追加が一番わかりやすいように思えます。

@luuthanhtuan
Copy link
Author

そうですね。
replaceSceneは、置き換えられたSceneを再利用しないシーン切り替えメソッドだと思っていましたが...

@jinnaiyuu
Copy link

replaceSceneの問題は非常に沢山のソースで言及されています。
html5gameengine.comで最低評価をされてしまっているのも、コメントを見るとこのメモリリークが大きな原因であるように思えます。redditでも質問が上がっていますし、jsのgame engineの比較の際にまず言及することでもあります。
replaceSceneが非再利用の為のメソッドだと認識されているのは間違いありません。他のjs game engineも、似たような形で再利用の為のメソッドと非再利用の為のメソッドを用意しています(Quintusなど)。再利用の為のメソッドとしてスタック以外のものを追加することは賛成です。しかし非再利用の為のメソッドもいずれかの形で準備するべきであり、APIの形からreplaceSceneをそれにするべきだと思います。

rtsan added a commit that referenced this issue Sep 11, 2014
@rtsan
Copy link
Collaborator

rtsan commented Sep 11, 2014

enchant.Scene#removeを追加しました。
イベントハンドラと子のNodeをすべて削除し、シーンスタックから自身を取り除きます。

@luuthanhtuan
Copy link
Author

replaceScene メソッドはこんな感じになりますでしょうか?

replaceScene: function(scene) {            
    this.popScene().remove();
    return this.pushScene(scene);
},

@jinnaiyuu
Copy link

rtsanさん, luuthanhtuanさん、ありがとうございます。
私のところではluuthanhtuanさんのようにreplaceSceneを書き換えて、それでメモリリークはなくなりました。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants