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

Remove: OjtPhonemestart / end 廃止 #807

Closed
3 tasks done
tarepan opened this issue Dec 3, 2023 · 2 comments · Fixed by #811
Closed
3 tasks done

Remove: OjtPhonemestart / end 廃止 #807

tarepan opened this issue Dec 3, 2023 · 2 comments · Fixed by #811

Comments

@tarepan
Copy link
Contributor

tarepan commented Dec 3, 2023

内容

要望: OjtPhonemestart / end 引数・属性の廃止

現在の OjtPhoneme は音素時刻を表現できる start / end 引数・属性を有する。
しかし現在の voicevox_engine は引数として時刻でなく単なる enumerate index を(place holder として)渡している。

OjtPhoneme(phoneme=p, start=i, end=i + 1)
for i, p in enumerate(phoneme_str_list)

また .start / .end 属性は外部から利用されず内部的に __eq__() で利用されているが、 #787 で示されたようにこの特殊 __eq__() も現在利用されていない(廃止PR #800 が進行中)。
すなわち、OjtPhonemestart / end 引数・属性は現在機能していない。
一方この引数はテストを煩雑化している。OjtPhoneme インスタンス生成のコード量が増加し、実利用されていない start/end 関連機能のテストにメンテコストを要している。

よって OjtPhonemestart / end 引数・属性廃止を提案します。

Pros 良くなる点

  • コード簡略化による見通し改善
  • テスト単純化によるメンテコスト減

Cons 悪くなる点

  • (不利用の)内部機能低下

実現方法

  • start / end 引数の廃止
  • .start / .end 引数の廃止
  • .start / .end 関連テストの廃止
  • .start / .end に強く依存する .__repr__() の廃止)

VOICEVOXのバージョン

0.14.10

OSの種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

#800 に依存しているため、本issue受け入れの場合、当該PRマージ後に着手します。

@github-actions github-actions bot added OS 依存:linux Linux に依存した現象 OS 依存:mac macOS に依存した現象 OS 依存:win Windows に依存した現象 labels Dec 3, 2023
@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 4, 2023

提案ありがとうございます!! 廃止の方向で良いのかなと思いました!!
(となるともうOjtPhonemeクラス自体いらなくなりそうな気もしてきました・・・!)

@tarepan
Copy link
Contributor Author

tarepan commented Dec 5, 2023

着手しました。

@tarepan tarepan removed OS 依存:mac macOS に依存した現象 OS 依存:linux Linux に依存した現象 OS 依存:win Windows に依存した現象 labels Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants