オブジェクトをどう更新するのが良いのか解りません。

質問

こんばんは、C++でのプログラミングについて質問させて頂きます。

現在DirectXライブラリを使って簡単なゲームを作っているのですが、ゲームのメインループの中で、各オブジェクトをどうやって更新するのが良いのかか解らず、アドバイスをもらえればと思い質問させて頂きました。

現在の更新方法は

for(Check_Num=1;Check_Num<PL_Limit;Check_Num++)
{
PL_Array[Check_Num]->Action();//各オブジェクトを更新
OBJ_Array[Check_Num]->Action();
FIRE_Array[Check_Num]->Action();
}
for(;Check_Num<OBJ_Limit;Check_Num++)
{
OBJ_Array[Check_Num]->Action();
FIRE_Array[Check_Num]->Action();
}
for(;Check_Num<FIRE_Limit;Check_Num++)
{
FIRE_Array[Check_Num]->Action();
}

Action関数には、表示を更新する、キー入力で移動する、接触したオブジェクトにダメージを与える、等といった各オブジェクトの動作が書かれています。

最初に定めたオブジェクトを生成できる上限の数まで、片っ端から全て更新していく、といった内容です。

また、オブジェクトの作成は

void Player::Shoot_FireBall()//ファイアボール
{
FireBall* FireBall1 = new FireBall(5,Ref_x(),Ref_y(),charge,"FIREBALL",Ref_angle(),5,0,40,40,4,true,"Action/FireBall1.png",6,3,2,1.0,true,true);
if(FIRE_num>=FIRE_Limit)
FIRE_num = 1;
FIRE_Array[FIRE_num++] = FireBall1;

}

といった関数を使い、特定のキーが入力された時に呼び出してオブジェクトを作成、上限に到達したら古いオブジェクトが入っていた配列を上書きして作る、といった内容です。

以上のようなソースコードを使ってやっていたのですが、このままオブジェクトの数(弾等の種類や敵の種類)が増えてくると、for文の中にその度に記述を追加していくとあまりにも長くなるような気がするのですが、この方法のままでやっていても大丈夫なのでしょうか・・？

後々効率が悪くなってくるような気がするのですが・・・、どうなのでしょうか、もし今の方法が悪いようでしたら、どう改善すれば良いのかを教えて頂ければと思います、宜しくお願いします。

回答No.1

BearCave · ベストアンサー

ＦＯＲループですべてのオブジェクトを更新するという考え方は合っています。ただいくつか気になる点があったので書いておきますね。

－更新にAction()という関数を呼び出していますが、一般的には「Update()」という名前を使います。（もちろん個人の自由なんですが、複数人が関わるプロジェクトですと、一般的なネーミングを使ったほうが好ましいので。）

－最初のＦＯＲループでＰＬ配列を全部更新、２つ目のＦＯＲループでＯＢＪ配列を全部更新、３つ目のＦＯＲループでＦＩＲＥ配列を全部更新、と分けた方が良いです。今の方針だとそれぞれのLimit数を後で変更した場合にループの配置を変えたりする必要が出て面倒ですし、バグも出易いです。

－ＦＯＲループのスタートが「１」になっていますが、「０」の間違いでしょうか？配列は「０」から始まるので。あと「FIRE_num=1;」の部分も同様に１からのスタートになっているので気になりました。

－FireBallクラスに「FireBall1.png」という画像ファイルを渡していますが、このクラスのコンストラクタで画像ファイルを読み込んでいるのでしょうか？だとするとFireBallを打つ度にファイルが読み込みされるので、スピード的にもメモリ的にも非常に効率が悪いです。普通、画像はすべてスタート時に読み込んでおいて、各オブジェクトには画像のポインターだけを渡します。FireBallが無数にあっても、画像は一つだけで済むようにしましょう。

－提示されているコードではFireBallを打つ度に「new」されていますが、使い終わったFireBallは「delete」されていないので、メモリリークが出ます。「FIRE_Array[FIRE_num] = FireBall1;」と上書きしてしまう前に「delete FIRE_Array[FIRE_num];」として上書き前のFireBallを削除しましょう。
もっと確実でシンプルな方法は、ゲームスタート時にFireBallを最大限分すべてnewして用意してしまうやり方です。クラスに「bool bShow;」というフラグを足して、これがtrueの時だけ更新したり描画したりするようにします。使い終わったらbShowをfalseにしてやり、新しいFireBallが必要な時はすでにfalseになっているオブジェクトを探してそれを再利用します。newとdeleteをする必要が無いので、メモリリークが起きません。

お礼

解答有難うございます、指摘された点の中で改善できそうなものは一通り改善してみました。

最初の３つについては改善できましたが、FireBallの画像とdeleteに関してはちょっとまだ試行錯誤中です、特にdeleteに関してはFireBallを生成するときに前回のFireBallをdeleteすることは出来たものの、プログムの最後に各配列をdeleteしようとするとエラーが出る、という状態ですが・・・、こちらはどうしても解決出来なければ、別途質問を投稿しようと思います。

また、FORループのスタートが1になっているのは、 いずれ列挙体名を使うかもしれないという考えと、列挙体を定義すると1から数字が当てはめられる、という記憶違いが原因です、以前から間違えていたのに気づいてはいましたが、修正しなければと思いつつも放置したままになっていたのでこの機会に直しました、有難うございます。

回答No.3

LongSecret

これは手厳しいご意見ですがｗ

でもいずれもまっとうなことですので、ためしにどういう形にするかイメージをまとめてみますと

仮に

PL_Array→players
OBJ_Array→objects (質問文だけだと私にはどういうものかわからないので、もっと適切な名前があったらそうしてください)
FIRE_Array→fires

Action→Update（あるいはフレーム単位の操作であれば私ならDoFrameとかにするかもわかりませんが）

とし

PL_Limitとかの型名をT(通常では int や unsigned intなどの事)とすると

for( T i=0; i<PL_Limit; ++i ) players[i]->Update();
for( T i=0; i<OBJ_Limit; ++i ) objects[i]->Update();
for( T i=0; i<FIRE_Limit; ++i ) fires[i]->Update();

ループとかの方はこんな感じでしょうか。

で、パフォーマンス（おもに速度）のために、画像の読み込みは1か所
元のコードはNULLチェックを行っていないので

FIRE_Limit数分だけ最初っからゲーム終了までは確保しっぱなしにして置くようなコードになってる、と仮に仮定すると

Shoot_FireBallでは

void Player::Shoot_FireBall(){

for( T i=0; i<FIRE_Limit; ++i ){
if ( fires[i]->IsUsed() ) continue;
fires[i]->SetMembersToReuse(更新するものだけの引数リスト);
return;
}

}

こういう感じになるかと思います。

※このコードだと、限界数使用中だったら発生しないとかそういうことになります。
可変長にしたかったらその管理をコード化してください。

ただし

＞このままオブジェクトの数(弾等の種類や敵の種類)が増えてくると、for文の中にその度に記述を追加していくとあまりにも長くなるような

普通だったら継承を使えば解決出来そうな状況に感じます。

宣言部だけ概要を示すとこんな感じですかね

基底クラス

class Object3D_Base {

protected:

float x, y, z;
bool is_used;

Object3D_Base();

public:

struct REUSE_FUNCDATA {
float x, y, z;
その他諸々
};

virtual void DoFrame() = 0; //純粋仮想関数にする
virtual void SetMembersToReuse( const REUSE_FUNCDATA* ) = 0; //ここは厄介ですがあくまで一例です

bool IsUsed() const { return is_used; }

virtual ~Object3D_Base();

};

派生クラス

class Player : public Object3D_Base {

void DoFrame() override { /* Playerの場合の更新内容 */ }
void SetMembersToReuse( const REUSE_FUNCDATA* fd ) override { /* Playerの場合のセット法 */ }

public:

Player();
~Player();

};

こうしておけば
生成の時だけPlayer（あるいはその他の派生クラス）でnewして
ポインタはObject3D_Baseとして統一できます。

Object3D_Base* p = new Player;
p->DoFrame();
delete p;

もしここまでで分かんない個所が一つでもあれば
C++の入門書を1冊程度は読むことをお勧めします。

ちなみに私が最初読んだのは
やさしい C++（高橋 麻奈著）でした。

かなり突っ込んだところまで学びたい場合は足りませんが
ある程度の基本的な部分は、非常に分かりやすく書かれていると思います。

お礼

解答有難うございます。

Forループを一つに纏める方法を知りたかったので非常に助かりました、まだオーバーライドについては何となくしか解っていないので、ゲームを作成しつつ並行して参考書の方も読むようにしてみようと思います。

何時も詳しく教えて頂きとても助かっています、ありがとうございます。

回答No.2

Tacosan

およそ #1 の通りですがちょっと蛇足:

・「オブジェクト指向」では, 一般にメッセージの名前は動詞を使い, オブジェクトの名前には名詞を使います. そうすれば, メッセージを送る式が
名詞 動詞 (以下目的語など)
という形になり, 英語の文章とそろうからです.

・for ループの変数ですが, ここははっきり言って意味のある名前は不要です. 特に #1 で指摘されているように「種類ごとに for を分ける」なら, 現状のように「あとで使いまわす」必然性もなくなるので for ループごとで変数を定義する方がかえって読みやすいはずです.

・PL_Array とかも今一つかなぁ. 「配列である」ことは明らかなので, わざわざ「Array」とつけることもないでしょう. まあ, 「OBJ_Array」の適当さに比べればましですが. 「Array」が不要なのは同じだけど, さらに「OBJ」という「一般的すぎて何を意味するのか全く分からない」言葉を使っているのが「ダメさ」加減を増している. 「オブジェクト」というなら, 「すべてのもの」が「オブジェクト」として扱えるよう工夫すべき.

・ちなみに「何を意味するのか分からない」という点では, check とか test とかを使った名前もだいたいダメと思っていい. もっと具体的に「何を『チェック』するのか」とか「どのようなことを『テスト』するのか」とかを考えて名前を付けるべき. あと, こいつらはえてして動詞だと思ってしまうので, 今のプログラムのように変数名として使うのも避けた方がいい.

お礼

変数名に関してのご指摘有難うございます。

極力何のための変数なのか解りやすい名前を付けるように心がけてみます。