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

[project-s] インポート周りの修正とリファクタリング #1614

Merged
merged 2 commits into from
Oct 19, 2023

Conversation

sigprogramming
Copy link
Contributor

内容

以下を行います。

  • IMPORT_MUSICXML_FILEの修正
    • tie(タイ)のstartstopが最初と最後の音符だけに設定されている場合でも正しく読み込めるように修正
  • リファクタリング
    • IMPORT_MIDI_FILEIMPORT_MUSICXML_FILEのリファクタリング
    • resolutiontpqnに変更
    • midinoteNumberに変更
    • 引数の順番を変更(tpqnが最後になるように)

関連 Issue

VOICEVOX/voicevox_project#15

その他

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

たぶんLGTM!!!

正直精査できてないのですが、いろいろ触ってみてとりあえず動いたので・・・!!!

Comment on lines +40 to 44
const tickToSecondForConstantTempo = (
ticks: number,
tempo: number,
tpqn: number
) => {
Copy link
Member

@Hiroshiba Hiroshiba Oct 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

これ全部同じ型だから順番間違えても気づきづらいの若干怖いかもですね!
好みなのですが、TypeScriptだったら確かこういう書き方もできたりします!

Suggested change
const tickToSecondForConstantTempo = (
ticks: number,
tempo: number,
tpqn: number
) => {
const tickToSecondForConstantTempo = ({
ticks,
tempo,
tpqn
}: {
ticks: number,
tempo: number,
tpqn: number
}) => {

こうすると実行側はこうやってキーを書かないといけなくなるので、間違いを減らしやすいかもです!

tickToSecondForConstantTempo({
  ticks: 1,
  tempo: 2,
  tpqn: 3
})

・・・まあ好みかなと!

@Hiroshiba Hiroshiba merged commit 645ab4b into VOICEVOX:project-s Oct 19, 2023
7 checks passed
@sigprogramming sigprogramming deleted the refactor_importer branch October 27, 2023 11:32
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

Successfully merging this pull request may close these issues.

2 participants