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

[Read Only]Sfx sounds #420

Closed
wants to merge 10 commits into from
Closed

[Read Only]Sfx sounds #420

wants to merge 10 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 16, 2018

A lot of changes after #372.
To do:

  • fix memory leaks, It's hard to determine when should/can I remove bd and ioBuffer.
  • ScriptSoundType isn't sound's position in sfx file, so sounds are mixed,
  • fix build,
  • setting type ScriptSound(opcodes 018d, 018e),
  • drop deprecated methods,
  • cleanup, div code in commits (done?),
  • stl algorithms
  • drop av_register_all()
  • opcode 03d7
  • remove mutexes,
  • cleanup 2.
  • rwfs::path

To do (after?)

  • updating listener position,
  • RE calculating sound volume,
  • some continuous sounds have break time (how long?),
  • resampling sfx.

As always feedback and criticism are welcome.

#edit1 implemented hardcoded table for translating between script and sfx numeration.
#edit2 plugged leaks.
#edit3 fixed build system.

@ghost ghost mentioned this pull request Apr 16, 2018
4 tasks
@ghost ghost mentioned this pull request Apr 30, 2018
@@ -116,6 +117,46 @@ char* LoaderSDT::loadToMemory(size_t index, bool asWave) {
return nullptr;
}

static int read_packet(void *opaque, uint8_t *buf, int buf_size) {
Copy link
Collaborator

@darkf darkf May 6, 2018

Choose a reason for hiding this comment

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

Can you make opaque a more useful name? Also, buf_size should be a size_t.

Copy link
Author

Choose a reason for hiding this comment

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

Api forces buf_size to to be int.

Copy link
Author

Choose a reason for hiding this comment

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

About upaque, I don't have better idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

is it a 'handle'?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is a callback the way it's implemented is already a good choice.
There should probably be a comment stating that this is an // ffmpeg XYZ callback (whatever XYZ is).

However, when casting the opaque you could give a more descriptive name than bd (although it's somewhat self documenting due to the cast)

@darkf
Copy link
Collaborator

darkf commented May 6, 2018

You don't use loadSound yet so I'm assuming this is still majorly WIP, but make sure its returned context is freed -- I wonder if it would not be better to use an RAII wrapper anyway.

Also needs a rebase ATM.

@ghost
Copy link
Author

ghost commented May 6, 2018

Please check: https://github.com/rwengine/openrw/pull/420/files#diff-e9cc6b7c0e9b40ae49ea18ed92518bfcR205
Using RAII is quite hard, all code of ffmpeg is written in C. So it's hard to balance between transparency and convenience of using. I mean AVFormatContext or custom RAII class.

ioBuffer = reinterpret_cast<uint8_t*>(av_malloc(ioBufferSize)); /// Will be freeded in SoundManager after buffering

bd = std::make_unique<bufferData>();
av_register_all();
Copy link
Contributor

@Lihis Lihis May 14, 2018

Choose a reason for hiding this comment

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

av_register_all() is deprecated?

Copy link
Author

Choose a reason for hiding this comment

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

Yes (in ffmpeg 4.0), it's also unneeded. To remove.

std::vector<int16_t> data;

size_t channels;
size_t sampleRate;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why these two are type of size_t?

Copy link
Author

Choose a reason for hiding this comment

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

WaveHeader is using uint.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, I missed that.

@ghost
Copy link
Author

ghost commented May 15, 2018

Two questions:
Should this pull also handle synchronization?

Should I prepare high level function for each opcode in SoundManager, without grabbing reference of sound? (Example of that high level function is playSound for 018c)


bool isPlaying() const;
bool isPaused() const;
bool isStoped() const;
Copy link

Choose a reason for hiding this comment

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

Misspelled?

Copy link
Author

Choose a reason for hiding this comment

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

yeah

@darkf
Copy link
Collaborator

darkf commented Jun 12, 2018

Synchronization of what, exactly?

Please leave opcode stuff to a follow-up PR as this is already large.

@ghost
Copy link
Author

ghost commented Jun 12, 2018

Yeah, pull is huge I'll try to extract/backport refactoring of code.

About synchronization, I was thinking about preparing ground for future resource manager. But it looks there's no need for now.

Filip Gawin added 8 commits June 20, 2018 19:23
Function loadSound returns
AVFormatContext with custom AVIOContext.
Most important changes:
- SoundManager manage all avaible types of sounds,
- each sound contains  "direct" methods,
(so we reduce duplicated searches on map by grabbing ref and
calling all needed methods on it (additionally for most opcodes
there high level wrapping methods)),
- binaural sound(although graabing parameters of listener should be improved),
- implementation all staff needed for handling sfx sounds.
Signed-off-by: Filip Gawin <filip.gawin@zoho.com>
Better, but there're some artifacts in case of
wav files.
@@ -17,6 +17,8 @@
#include <data/CutsceneData.hpp>

#include <boost/algorithm/string/predicate.hpp>
#include <string>
Copy link
Author

Choose a reason for hiding this comment

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

To remove.

#include "audio/Sound.hpp"

/// Script is using different numeration of sounds
/// than postion index in sfx file.
Copy link
Author

Choose a reason for hiding this comment

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

position

int sfx;
int range;
};
/// Hardcoded array for translation script index into sfx index
Copy link
Author

Choose a reason for hiding this comment

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

translating


private:
std::vector<int16_t> data;
/// Setting velocity of velocity for openAL.
Copy link
Author

Choose a reason for hiding this comment

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

of listener

@@ -1,6 +1,7 @@
#ifndef _RWENGINE_SOUNDMANAGER_HPP_
#define _RWENGINE_SOUNDMANAGER_HPP_

#include <algorithm>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should include only what you use in that file. So algorithm is better included in the cpp file.

Copy link
Author

Choose a reason for hiding this comment

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

IIRC some small function in headers are using it.

@ghost ghost mentioned this pull request Jun 21, 2018
@ghost ghost changed the title [Wip]Sfx sounds [Read Only]Sfx sounds Aug 4, 2018
@ghost
Copy link
Author

ghost commented Aug 13, 2018

Everything useful has been extracted. ;)

@ghost ghost closed this Aug 13, 2018
@ghost ghost deleted the sfxSounds branch August 16, 2018 11:50
This pull request was closed.
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.

4 participants