横スクロールアクションでのキャラ移動のリファクタリングについて

[WhiteRabbit]

DXライブラリを用いて、横スクロールアクションを製作しています。
キャラ移動をサイトを参考に、ベルレ法で書きました。
void player::update()の中が、何回も衝突判定を呼び出して割り算したりして
もっと効率よく書きたいと思ってます。が、どうすればいいのか全然考えつきません。
（効率よく書きたいというのは、冗長性を少なくしたいということです。）
あれこれいろいろなサイトを参考にしてますが、どうしても思いつきません。
本当はそれでも自分で考えなきゃ勉強にならないことはわかってますが
あまりにもここのリファクタリングで時間を書けているので掲示板で質問させていただきました。
シーンマネージャークラス等もあるので、キャラ移動の処理に関係するファイルだけ抜粋して載せました。

コード:

//プレイヤークラスのヘッダファイル
#pragma once
#include <Dxlib.h>
#include <Sources\Data\Map.h>
class Player
{
public:
	Player();
	~Player();
	void Update();
	void Draw();
	int getpX() const;
	int getpY() const;
private:
	int player;
	int Width = 32;
	int Height = 32;
	int X, X_prev, X_move;
	int Y, Y_prev, Y_move;
	int f;
	bool canJump;
	

	Map* map;
};

//プレイヤークラスのソースファイル
#include "Player.h"

Player::Player() : X(320), Y(240), X_prev(320), Y_prev(240), X_move(0), Y_move(0), f(2)
{
	player = LoadGraph("Resources/player.png");
	map = Map::Instance(); //マップ管理（シングルトン）
	map->Initialize();
}

Player::~Player()
{
}

void Player::Update() {

	//重力の設定(Verlet積分)
	Y_move = (Y - Y_prev) + f;
	if (Y_move > 31) Y_move = 31;
	Y_prev = Y;
	Y += Y_move;
	f = 1;

	//天井衝突判定
	if (map->collisiton_Block_up(X, Y)) {
		Y = Y / 32 * 32 + 32;
	}

	//床衝突判定
	if (map->collisiton_Block_down(X, Y)) {
		Y = Y / 32 * 32;
		canJump = true;
	} else {
		canJump = false;
	}

	//左右移動
	if (CheckHitKey(KEY_INPUT_RIGHT) || CheckHitKey(KEY_INPUT_D)) X += 2;
	if (CheckHitKey(KEY_INPUT_LEFT) || CheckHitKey(KEY_INPUT_A)) X -= 2;

	//壁衝突判定(右側)
	if (map->collisiton_Block_right(X, Y)) {
		X = X / 32 * 32;
	}

	//壁衝突判定(左側)
	if (map->collisiton_Block_left(X, Y)) {
		X = X / 32 * 32 + 32;
	}


	//ジャンプ処理(Verlet積分)
	if (CheckHitKey(KEY_INPUT_SPACE) && canJump) {
		f = -15;
	}
}

void Player::Draw() {
	DrawGraph(X, Y, player, TRUE);
}

//マップクラスのヘッダファイル
#pragma once
#include <vector>

class Map //シングルトン
{
public:
	static Map* Instance();
	void Initialize();
	void Draw();
	bool collisiton_Block_up(int pX, int pY);
	bool collisiton_Block_down(int pX, int pY);
	bool collisiton_Block_left(int pX, int pY);
	bool collisiton_Block_right(int pX, int pY);
private:
	//Mapをシングルトンにするため下記３つをprivateに
	Map(); //デフォルトコンストラクタ
	Map(const Map& r) {} //コピーコンストラクタ
	Map& operator=(const Map& r) {} //代入演算子のオーバーライド
	std::vector<std::vector<int>> originalmap{
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 },
		{ 0,0,0,0,0,0,0,1,1,1,0,0,0,0,0,0,0,0,0,0 },
		{ 0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0 },
		{ 0,1,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,1,0 },
		{ 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 },
		{ 1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1,1 },
	};
	std::vector<std::vector<int>> map;
	int map_height = originalmap.size();
	int map_width = originalmap.front().size();
	int chip[2];
	int chipSize = 32;
};


//マップクラスのソースファイル
#include "Map.h"
#include <DxLib.h>
#include <Sources\Character\Player.h>

Map::Map()
{
}

Map* Map::Instance() {
	static Map instance;
	return &instance;
}

void Map::Initialize() {
	map.clear();
	map = originalmap;
	LoadDivGraph("Resources/chip.png", 2, 2, 1, chipSize, chipSize, chip);
}

void Map::Draw() {

	for (int y = 0; y < map_height; y++) {
		for (int x = 0; x < map_width; x++) {
			DrawGraph(x * chipSize, y * chipSize, chip[map[y][x]], TRUE);
		}
	}
}

bool Map::collisiton_Block_up(int pX, int pY) {
	return map[pY / 32][pX / 32] == 1 || map[pY / 32][(pX + 31) / 32] == 1;
}

bool Map::collisiton_Block_down(int pX, int pY) {
	return map[(pY + 31) / 32][pX / 32] == 1 || map[(pY + 31) / 32][(pX + 31) / 32] == 1;
}

bool Map::collisiton_Block_left(int pX, int pY) {
	return map[pY / 32][pX / 32] == 1 || map[(pY + 31) / 32][pX / 32] == 1;
}


bool Map::collisiton_Block_right(int pX, int pY) {
	return map[pY / 32][(pX + 31) / 32] == 1 || map[(pY + 31) / 32][(pX + 31) / 32] == 1;
}

[usao]

> void player::update()の中が、何回も衝突判定を呼び出して割り算したりして

何度もやっているのが無駄だと思うのであれば，一度だけやるようにすればよいのではないでしょうか．

例えば，何度も Xを32で割っているのが気になるなら，Xを32で割る処理群の手前で

コード:

int X_div_32 = X/32;

とか書いて，以降の処理でこの値を使えば，割り算の回数が減りますよね．

本質的なデータであるマップのデータ　と　描画の都合でしかない絵のサイズ(=32)　とが
ごちゃまぜになって扱われているせいで，各所に32という値が散在する結果になってしまっているようにも感じます．

（質問内容と直接的に関係ない，他の事柄には触れません）

[WhiteRabbit]

返答ありがとうございます。
自分で考えてるときに
一度処理前にまとめてそのように変数を書きましたが
そうしてしまうと、移動がめちゃくちゃになりちゃんと動作しなくなってしまいました…。

[かずま]

負にならない変数を int でなく、unsigned int で宣言すると、
unsigned x; で x / 32 は、x >> 5 というコードになり、
高速の 1命令になります。
int x でも割り算のコードは出ず、次のようになります。

コード:

    if (x < 0) x += 31;
	x >> 5;

[ISLe]

そもそも、その場所からどかなければならない理由は、Playerにとっては知らなくて良いことです。
Playerのメソッドの中で、マップとのあたり判定を行っていること自体が不自然です。

Playerは、どの方向にどれだけどけばよいか(あるいはそれを求めるために必要十分な情報)を得て、その情報のみに従って移動先を決めればよいというのがわたしの考え方です。

わたしなら、Player::Updateの外で、プレイヤーと壁(やその他の障害物など)の当たり判定を行い、例えばRECT構造体に上下左右のめり込み量を設定。Player::Update内でそれを取得して、挙動を決定するようにします。

既に移動できないことが分かっている状態で、ユーザー操作による移動をキャンセルするなどの対応もできます。

ゲームデザインによっては、動く壁・通り抜けられない敵などの存在によって、上下あるいは左右同時に挟まれる可能性があるので、上下左右すべての情報を持っておくと汎用に使えます。

Player::Update内で取得する方法は、コンテキストマネージャーなどの管理オブジェクトを通して行います。
Player::Updateの引数に直接記述するのは、Playerのヘッダの変更頻度を高くして余分なビルドのコストを増やします。

あとから敵の攻撃によるノックバックなどを追加するとき、Playerのコードに一切影響を与えることなく実装することもできます。

[WhiteRabbit]

みなさん回答ありがとうございました。

「その場所からどかなければならない理由は、Playerにとっては知らなくて良いことです」
たしかにそうですね…。

クラス設計をする上で、もっと現実的に考えてみれば、ここはこのクラスが持つとか、逆にいらないとかわかるのでそう考えるのが大切ですね…。

その考えで、リファクタリングしていきます！

どうもありがとうございました！！