yumetodo commented 7 years ago

キー操作関連をクラス化(ported form takeokunn/DoNotPushGame )
MessageBox系関数にKeyStateクラスを渡し、確実に入力がflushされるように( try to resolve #20 )
負荷軽減

基本方針

キー操作関連はすべてKeyStateクラスを介して行う
MessageBoxでのこるキー判定を削除するためにこれをラップする関数にKeyStateクラスを渡す
キー操作をする関数はすべて引数にKeyStateクラスを受け取る
const KeyState&と引数の型がなっている場合はKeyStateクラスにキャッシュされたキー入力を見る
KeyState&と引数の型がなっている場合はキー操作との同期が発生するが、関数を抜けるまでにflushとupdateがされた状態で関数を抜ける

Issue

[x] std::string::operator[]のassertに引っかかる => 55f4a291f7f0c26c906b3a0049e5375d44698288, 04c420198596f7d7ae8334343d969a3da5786b67
[x] マウス操作時にキー操作が生きている
[x] キー操作が速すぎてついていけない
[x] F4キーでの既読スキップが実行できない
[x] ゲームメニューから既読スキップを実行すると、既読スキップ（skip_auto = Skiptype::skip;）は有効になるが、ゲームメニューのまま
[x] ゲームメニューから既読スキップを実行後、ゲームに戻ると既読スキップが実行される
[ ] マウス操作時に、各種Fキーでのショートカットキーが利用できない

ref:

yumetodo commented 6 years ago

迷走した結果、どうにかきれいに収まりそうな解決策ができたのでcommitしました。肝は 4601907 と 0909ad2 です。

マウス操作時にキー操作が生きている問題については 4601907 で多少改善しましたが、根本的にはConfigData.mouse_key_moveをちゃんと見ていない箇所があるのでそれを治す必要があります(この点masterも壊れてるぽい)

yumetodo commented 6 years ago

@S-H-GAMELINKS まあしかし方向性としてはこれでいいかなと思うんですが、どうでしょうか？ちょっと試してみてください・・・。(現状キーの速度速すぎ問題対応はタイトル画面とコンフィグ画面のみ)

S-H-GAMELINKS commented 6 years ago

お疲れ様です。

マウス操作時の挙動はこちらでも確認しました。ちょっとコミットを確認しながら、挙動の把握をしておきますね

S-H-GAMELINKS commented 6 years ago

実際に動作させている中で、assert に引っかかっている場所が見つかりました。

SCRIPT_OUTPUT() の assert(std::size_t(CP + 1) < String[SP].size());　が引っかかっているようです。

これは、インクリメントされたCP + 1 がString[SP] 内の要素数より大きくなった時にassertに引っかかるということでいいんですよね？

だとしたら、実際のコードとしては

assert(std::size_t CP < String[SP].size());

になるかと

現状ですと、各行末に到達するたびにassertに引っかかると思われます

追記：

これ、以前も引っかかってたところですね……

ちょっと調べておきます

さらに追記：

手元のmasterでは、以下のようになっているので問題ないようです。

assert(std::size_t(CP + 1) <= String[SP].size());

1dd5157 のコミットで assert(std::size_t(CP + 1) < String[SP].size()); となったようです。

S-H-GAMELINKS commented 6 years ago

マウス操作時にコンフィグ画面にて音量などを調節した際に以下のような現象が確認されました。

１：BGM音量にカーソルを合わせる

２：クリックして音量を変更

３：変更した音量が元に戻る(ex:100→90→100といった感じ)

ちょっと処理部分のコード確認しておきますね

追記：

とりあえず、該当箇所のコードを見てみました。

が、ちょっとよくわからない感じですね……

数値が90までしか減りませんし、マウス/キー判定で音量などをプラスしているif文がtrueとなっているのかも……？

ex:

    //コンフィグ(BGM音量調節)
    void BGM_VOL_CHANGE(const KeyState& key) {
        //ＢＧＭ音量調整
        if (GAME_y == game_menu_base_pos_y && (key.right() || ((GetMouseInput() & MOUSE_INPUT_LEFT) != 0))) {  ←　この式ずっと true になっている……？
            ConfigData.bgm_vol += 10;
            ConfigData.bgm_vol_count += 1;
            if (ConfigData.bgm_vol_count >= 10) {
                ConfigData.bgm_vol = 100;
                ConfigData.bgm_vol_count = 10;
            }
        }

        if (GAME_y == game_menu_base_pos_y && (key.left() || ((GetMouseInput() & MOUSE_INPUT_RIGHT) != 0))) {
            ConfigData.bgm_vol -= 10;
            ConfigData.bgm_vol_count -= 1;
            if (ConfigData.bgm_vol_count <= 0) {
                ConfigData.bgm_vol = 0;
                ConfigData.bgm_vol_count = 0;
            }
        }

    }

さらに追記：

当該現象が確認できなくなりました……

もうちょっと調べてみますね

S-H-GAMELINKS commented 6 years ago

とりあえず、現状の方向性としてはこのままで問題ないと思います。

S-H-GAMELINKS commented 6 years ago

キー操作時のずれ、確認致しました。

S-H-GAMELINKS commented 6 years ago

先ほどの、コンフィグ画面でのBGM音量変更の挙動ですが

Pull したところ該当する挙動が確認されなくなりました。

恐らく、動作確認中に此方が変更していた箇所が影響していたようです。

申し訳ない。

yumetodo commented 6 years ago

BGMの件了解です。とりあえずassertの件は修正してみました。

yumetodo commented 6 years ago

キー操作時のずれ、確認致しました。

これどうしようかな・・・。キー離した次のループだけはキー受け付けないようにしてしまおうか・・・

S-H-GAMELINKS commented 6 years ago

もしくは、'return n' を 'return 0' に変更してしまってキー移動の反応をよくするか、ですかね……？

キーの反応が悪くなる要因がわかっていれば対処できそうですが……（私はわかってないので、対策が浮かびませんが……）

yumetodo commented 6 years ago

return 0;にすると、wait_key_changeで待っている間の最後の瞬間に押されているキーに反応するので、キーの反応が悪くなります。

S-H-GAMELINKS commented 6 years ago

あ、なるほど。

ありがとうございます。

とすると、@yumetodoさんのキー受付をしないという手がよさそうですね

S-H-GAMELINKS commented 6 years ago

とりあえず、既読スキップの SKIP_READ_CHECK は機能しているようですね……

試しにゲームメニューから既読スキップを実行したところスキップされていました。

ただし、ゲームに戻るを押してからでした。

ですので、リファクタリングしていく中で、ゲームメニューループから抜け出さなくなっている模様です。

ファンクションキーから既読スキップを実行した場合はスキップがはじまりませんでしたので、

こちらはまた別な要因が絡んでいそうです

yumetodo commented 6 years ago

@S-H-GAMELINKS ええっとちょっと発見できているバグを箇条書きしていただけると・・・(masterで発現するか含めて)

S-H-GAMELINKS commented 6 years ago

えーと、今確認できる範囲ですと

１：F4キーでの既読スキップが実行できない

２：ゲームメニューから既読スキップを実行すると、既読スキップ（skip_auto = Skiptype::skip;）は有効になるが、ゲームメニューのまま

３：ゲームメニューから既読スキップを実行後、ゲームに戻ると既読スキップが実行される

４：マウス操作時に、各種Fキーでのショートカットキーが利用できない　　また、エスケープキーなども利用できない模様

以上の４点ですね。

追記：

既読スキップ関連はmaster でも発現しています。

S-H-GAMELINKS commented 6 years ago

とりあえず、２と３に関しては以下のようにコードを修正することで対処できました。

//既読スキップ判定
void SKIP_READ_CHECK(KeyState& key) noexcept {
    const SkipDataConv* conv = reinterpret_cast<const SkipDataConv*>(&TextIgnoredFlags);
    //既読データ読み込み時の判定
    if (IDYES == MessageBoxYesNo("既読スキップを実行しますか？", key, KeyState::Executor::flush_update) && 0 < EndFlag && EndFlag <= countof(conv->arr) && 1 == conv->arr[EndFlag - 1]) {
        skip_auto = Skiptype::skip;
        GAMEMENU_COUNT = true;
        //サウンドノベル風描画時の処理
        SOUNDNOVEL();
        //ウインドウ風描画時の処理
        WINDOWNOVEL();
    }
    //ショートカットキー時の事後処理
    SHORTCUT_KEY_DRAW();
}

どうも、ゲームメニューを抜けるための GAMEMENU_COUNT = true がなかったのが原因だったようです

yumetodo commented 6 years ago

そもそもGAMEMENU_COUNTなんて変数じゃなくて例外投げつけたい衝動に駆られてきたけどますます差分がでかくなるので自重しよう・・・。

S-H-GAMELINKS commented 6 years ago

ですよね……

S-H-GAMELINKS commented 6 years ago

１に関しては下記のコードで修正できました。

    //既読スキップ
    if (EndFlag != 99 && key == KEY_INPUT_F4) {
        SHORTCUT_KEY_FLAG = 1;
        GAMEMENU_COUNT = false;
        SKIP_READ_LOAD();
        SKIP_READ_CHECK(key);
    }

SKIP_READ_LOAD(); が無く、既読スキップ用データが読込まれていなかったことが原因のようです

S-H-GAMELINKS commented 6 years ago

４に関してはキー操作をKeyStateに渡していますから、マウス操作時には使えなくなるのは当然でしたね……

ショートカットキー、エスケープキーでのゲーム終了などは従来通りの CheckHitKey で処理しましょうか……？

yumetodo commented 6 years ago

ショートカットキー、エスケープキーでのゲーム終了などは従来通りの CheckHitKey で処理しましょうか……？

ScopedKeyStateActivatorクラスを返すメンバ関数をKeyStateクラスに追加して、そのデストラクタが呼ばれるまでフラグを倒しておいて、倒れている間はConfigData.mouse_key_moveを無視するようにしましょう(RAII)。

S-H-GAMELINKS commented 6 years ago

ああ、その方法がありましたか……

それなら、ショートカットキー機能も共存できますね

S-H-GAMELINKS commented 6 years ago

キー操作のズレ＆スピードに関してなんですが、下記のようにコードを書き換えてみた所正常なスピードでずれもなく動作できたと思われます。

    //セーブ画面(キー操作)
    void SAVEDATA_KEY_MOVE(KeyState& key) {

        if (key.down()) {
            SAVE_y = (SAVE_y == (save_buttom_y)) ? save_base_pos_y : SAVE_y + save_move_unit;
        }

        if (key.up()) {
            SAVE_y = (save_base_pos_y == SAVE_y) ? save_buttom_y : SAVE_y - save_move_unit;
        }
        key.flush();
    }

もとのコードは以下のようになっていました

    //セーブ画面(キー操作)
    void SAVEDATA_KEY_MOVE(const KeyState& key) {

        if (key.down()) {
            SAVE_y = (SAVE_y == (save_buttom_y)) ? save_base_pos_y : SAVE_y + save_move_unit;
        }

        if (key.up()) {
            SAVE_y = (save_base_pos_y == SAVE_y) ? save_buttom_y : SAVE_y - save_move_unit;
        }
    }

たぶん、これで他なキー操作のズレ＆スピードの調整ができるのではないかと思います。

yumetodo commented 6 years ago

@S-H-GAMELINKS 大変長らくおまたせしました。

191562b にて、当初の

キー離した次のループだけはキー受け付けないように

を実装してみました。

yumetodo commented 6 years ago

ついでにfmtlib/fmtのバージョンを上げています

yumetodo commented 6 years ago

なおClang CodeGenでコンパイルしようとすると

In file included from C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\exception:7:
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\type_traits(898,47): error : '_Ty' does not refer to a value
                : bool_constant<__is_trivially_destructible(_Ty)>
                                                            ^
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\type_traits(896,16) :  note: declared here
template<class _Ty>
               ^
C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\VC\Tools\MSVC\14.12.25827\include\type_traits(899,2): error : expected class name
        {       // determine whether _Ty has a trivial destructor
        ^
2 errors generated.

エラーが出るが、これは https://bugzilla.mozilla.org/show_bug.cgi?id=1423307 https://stackoverflow.com/questions/47680258/clang-c2-on-visual-studio-15-5-bool-constant-is-trivially-destructible-ty-e のとおり既知の不具合。多分Clang CodeGenのclangのバージョンが~~古いので~~__is_trivially_destructibleというMSVC 1910で追加されたコンパイラ拡張に対応していない

S-H-GAMELINKS commented 6 years ago

お疲れ様です

コミットと実機での動作確認いたしました。

こちらで問題ないと思います。どうぞ、よろしくお願いいたします。

yumetodo commented 6 years ago

もうすでにどこが問題の描画ループかわからなくなっているの図(頑張って探します・・・

S-H-GAMELINKS commented 6 years ago

たしか、KEY_MOVE()みたいな関数の後に描画ループがあるかと思います

yumetodo commented 6 years ago

うーん、なんでセーブ画面ループがチカチカするのか分からん・・・。

それはそうとCreateSaveDataの

    static int CreateSaveData(int* SaveSnapHandle, const char* Message, const char* ImagePath, const char* SaveDataPath, KeyState& key) {
        //TODO: このif文は呼び出し元のループに持っていく
        if (IDYES == MessageBoxYesNo(Message, key, KeyState::Executor::none, KeyState::Executor::flush_update)) {
            //セーブデータ１用のスクリーンショット取得変数
            *SaveSnapHandle = 1;

            //選択肢画面でのセーブ処理
            if (SAVESNAP_CHOICE != 0) {
                scoped_screen screen(DX_SCREEN_BACK);
                //TODO: これはなんなのか突き止める。SAVESNAP_CHOICEってフラグなのかハンドルなのか・・・
                DrawGraph(0, 0, SAVESNAP_CHOICE, TRUE);
                SaveDrawScreenToPNG(0, 0, 640, 480, ImagePath, 0);
                SAVESNAP_CHOICE = 0;
                *SaveSnapHandle = 0;
            }
//後略

DrawGraphにSAVESNAP_CHOICEが渡っているのは何かがおかしい。

S-H-GAMELINKS commented 6 years ago

これは、ハンドル兼フラグで使いまわししてるものになりますね……

これまでの開発過程で、以下のような手順で選択肢画面でのセーブを実装してました１：選択肢画面でのセーブを実行２：選択肢画面でのみスナップショットをとり、SAVESNAP_CHOICEに渡す。３：セーブ時にSAVESNAP_CHOICE が空でなければ、それをDrawGraphに渡してスクリーンショットを取る

そういう使い方をしている変数になりますね……

yumetodo commented 6 years ago

うへぇ・・・

変数分離します。

yumetodo commented 6 years ago

画面チカチカ問題とSAVESNAP_CHOICEの件、直しました。

マウス操作時にキー操作が生きている
キー操作が速すぎてついていけない

は治ったはず、

マウス操作時に、各種Fキーでのショートカットキーが利用できない

はどうやって動作検証すれば良いんでしたっけ？これで全部のバグが潰れていることを祈りたい・・・

S-H-GAMELINKS commented 6 years ago

お疲れ様です。また確認しておきますねー

マウス操作時でのFキー操作は、「マウス操作を有効にした状態で、ゲーム画面にて各Fキーを押す」ことで確認できると思います

S-H-GAMELINKS commented 6 years ago

確認しました。

・マウス操作時のキー操作が活きている・キー操作のスピード

上記二点は解決されていますねー

ただ、各Fキーでの操作はやはり生きていない状況ですね(ESCAPEキーも同様)

S-H-GAMELINKS commented 6 years ago

あと、SCREEN_CLEAR() を取り除かれていましたが、以下の ClearDrawScreen() とで二重に画面のクリア処理が行われていたことがチカチカする原因のようです。

        auto normal_con_f = []() -> bool {
            return -1 != ProcessMessage() && 0 == ScreenFlip() && 0 == ClearDrawScreen();
        };

ですので、こちらをこのように変更し

        auto normal_con_f = []() -> bool {
            return -1 != ProcessMessage() && 0 == ScreenFlip();
        };

かつ、ループ内に SCREEN_CLEAR() を設置したところチカチカすることはありませんでした。

それと、こららのSCREEN_CLEAR() を取り除いたことでセーブ後のゲーム画面への戻り処理などで画像などが表示されない(恐らく、ClearDrawScreen()にてクリア処理されている)ようです。

おそらく、セーブ画面とロード画面、それとゲームメニューからゲーム画面に戻るところ以上の三点で、画像類がクリアされているようです。

yumetodo commented 6 years ago

それと、こららのSCREEN_CLEAR() を取り除いたことでセーブ後のゲーム画面への戻り処理などで画像などが表示されない(恐らく、ClearDrawScreen()にてクリア処理されている)ようです。

ループ中でクリアするのはダブルバッファリングするようにした都合上避けたいので、SCREEN_CLEAR()を元の位置に復活する以外の道を探りたいのですが・・・

S-H-GAMELINKS commented 6 years ago

分かりました。

ちょっと、ソースコードとにらめっこしてきます

yumetodo commented 6 years ago

ただ、各Fキーでの操作はやはり生きていない状況ですね(ESCAPEキーも同様)

これについて前に

ScopedKeyStateActivatorクラスを返すメンバ関数をKeyStateクラスに追加して、そのデストラクタが呼ばれるまでフラグを倒しておいて、倒れている間はConfigData.mouse_key_moveを無視するようにしましょう(RAII)。

と言ったまま何もしていないのを思い出した・・・

S-H-GAMELINKS commented 6 years ago

では、これであれば問題ないかと

        auto normal_con_f = []() -> bool {
            if(GAMEMENU_COUNT == false)
                return -1 != ProcessMessage() && 0 == ScreenFlip() && 0 == ClearDrawScreen();
        };

ループを抜けるときにはGAMEMENU_COUNT = true とされていますので、ループを抜けるときだけ画面のクリア処理が走らなくなります。

yumetodo commented 6 years ago

あー、そのフラグが噛んでくるのか・・・。本格的にその辺は例外を使うように書き換えたい・・・。

とりあえず当座その方向で対処します。

yumetodo commented 6 years ago

描画ループ脱出フラグチェックを継続条件判定の中で最初にやるように変更しました。

S-H-GAMELINKS commented 6 years ago

コミット拝見しました

確かに、これならClearDrawScreenが後に回りますから問題無いですね

あとで、動作確認しておきますね

S-H-GAMELINKS commented 6 years ago

動作確認致しました。

問題なく動作しております。

S-H-GAMELINKS commented 6 years ago

ver3.20以前のLINKSの挙動を確認していたのですが、マウス操作時のゲームループ中にエンターキーなどのキー操作が使えるようにしていました。

ですので、ゲームループ中のkey.update() をオーバーロードしたメンバ関数：key.update(std::uint32_t flag)のようなものを実装してキー操作が可能にしてもいいかもしれませんね。

とはいえ、その場合は「マウス操作時にエンターキーなどを誤って押してしまい、誤操作を招く」という状態が残りますが……

yumetodo commented 6 years ago

やっぱりキー操作のたぐいを統一的に管理していないのがバグの要因であったり可読性を下げている要因だと思うので、現在KeyStateクラスにキー操作を登録できる(なんちゃってイベント)ようにできないか検討中です(boost::container::small_vectorがC++標準にほしい)。

イメージとしては


void foo(KeyState& key)
{
    auto scoped_key_bindings = key.make_scoped_key_bindings(KeyBind::overwrite, KeyState::always_active, {
        { { KEY_INPUT_RETURN }, [](){ /* on enter key pushed */ } },
        { { KEY_INPUT_LSHIFT, KEY_INPUT_UP }, [](){ /* on lshift + ↑ pushd */ } }
    });
    for( /* 描画ループ*/) {
        //なんか
    }
}

S-H-GAMELINKS commented 6 years ago

確かに、これはいいですねー

yumetodo commented 6 years ago

問題は、描画ループ内から別の描画ループ内に行っているケースが散見されるので、その場合親ループで有効だったkeybindingが子ループに強制的に引き継がれるので、もし親ループでは有効にしたいけど子ループでは無効にしたい、とか言うのがある場合はだいぶ差分がでかくなってしまう。

そういうケースってありますかね・・・？

S-H-GAMELINKS commented 6 years ago

元々そういった細かな場合分けは行っていませんので、そういったケースらしいものはないと思われますね。

近いところだと、マウス操作時に各種FキーやESCキーを有効にしておいてあるケースですかね。でも、この場合は親ループ（ゲームのループ）と子ループ（ゲームメニューのループ）どちらもでESCキーを使うようにしていますし、なによりFキー類は子ループでは使う関数もありませんし……

ですので、把握できる範囲ではそういったケースはありませんね。

S-H-GAMELINKS / Novel.Game.Engine.LINKS

feat: use KeyState class instead of CheckHitKey #28

基本方針

Issue

20

21