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

Adjusts to KHR_audio PR recommendations to split audio out #88

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

antpb
Copy link
Contributor

@antpb antpb commented Jun 4, 2022

This PR adjusts the schema structure to account for feedback provided in KHR_Audio proposal:

KhronosGroup/glTF#2137 (comment)

{
  "emitters": [
    {
      "name": "Positional Emitter",
      "type": "positional",
      "gain": 0.8,
      "inputs": [0, 1],
      "positional": {
        "coneInnerAngle": 6.283185307179586,
        "coneOuterAngle": 6.283185307179586,
        "coneOuterGain": 0.0,
        "distanceModel": "inverse",
        "maxDistance": 10.0,
        "refDistance": 1.0,
        "rolloffFactor": 0.8
      }
    }
  ],
  "sources": [
    {
      "name": "Clip 1",
      "gain": 0.6,
      "playing": true,
      "loop": true,
      "audio": 0
    },
    {
      "name": "Clip 2",
      "gain": 0.6,
      "playing": true,
      "loop": true,
      "audio": 1
    }
  ],
  "audio": [
    {
      "uri": "audio1.mp3"
    },
    {
      "bufferView": 0,
      "mimeType": "audio/mpeg"
    }
  ]
}

@antpb
Copy link
Contributor Author

antpb commented Jun 4, 2022

in the PR Audio was structured as

  "audio": [
    {
      "uri": "audio1.mp3"
    },
    {
      "bufferView": 0,
      "mimeType": "audio/mpeg"
    }
  ]

Are those objects supposed to be split out? In the readme I mocked it like

      "audio": [
        {
          "uri": "audio1.mp3",
          "bufferView": 0,
          "mimeType": "audio/mpeg"
        }
      ]

Once this is clear, I can update the PR and get the sample asset matching.

@robertlong
Copy link
Member

in the PR Audio was structured as

  "audio": [
    {
      "uri": "audio1.mp3"
    },
    {
      "bufferView": 0,
      "mimeType": "audio/mpeg"
    }
  ]

Are those objects supposed to be split out? In the readme I mocked it like

      "audio": [
        {
          "uri": "audio1.mp3",
          "bufferView": 0,
          "mimeType": "audio/mpeg"
        }
      ]

Once this is clear, I can update the PR and get the sample asset matching.

You can use the glTF.image schema as a reference for audio, it's almost the same.

@robertlong
Copy link
Member

Also one more change we probably want to make is to change input to sources. This is closer in naming to the audioSources and makes it a bit more obvious.

@@ -11,6 +11,30 @@
"type": "object",
"$ref": "source.schema.json"
},
"playing": {
Copy link
Member

Choose a reason for hiding this comment

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

Also I think we wanted to change playing back to autoPlay

@antpb antpb changed the title Adjusts to KRH_Audio PR recommendations to split audio out Adjusts to KHR_audio PR recommendations to split audio out Jun 9, 2022
@oveddan
Copy link

oveddan commented Jun 10, 2022

Awesome work and I can see myself using this in my project!

Some questions:

  • With this extension, how would you specify the actual position of the audio? Is it dependent on the position of the parent element? Or is there a position and rotation attribute?

Suggested future improvements:

  • Being able to filter audio channels in each emitter, i.e. only play a single audio channel
  • A flag indicating that playback position should be synchronized across user sessions.

@fire
Copy link
Contributor

fire commented Jun 16, 2022

@robertlong Since you are an implementer of KHR_audio.

We discussed we can choose different horizons of updates. There were opinions of a few days versus stabilization. Wanted to poll.

@antpb requested review the structure of the KHR_audio specification.

Copy link
Contributor

@fire fire left a comment

Choose a reason for hiding this comment

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

We didn't want to break existing implementations because of the audio clips structure, but we didn't note any other changes.

Want to try a new strategy where we gather opinion via approvals and request changes to disconnect the approval process from merging.

Since this was discussed in the gltf meeting and there was consensus that it was approved. I wanted to leave the window open and then merge on Friday noon tomorrow PDT time.

@antpb
Copy link
Contributor Author

antpb commented Jun 16, 2022

three omi to be updated after this is merged

@fire fire merged commit e5c279c into omigroup:KHR_audio Jun 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants