FEAT(plugin-api): Add function to mumble api to obtain current positional audio data provided by another plugin

[JordanPlayz158]

This is a draft PR (not in a working state), the reason for the expansion is to allow for plugins that perform changes to the users settings (configurable) in response to context or identity information provided by positional audio games (without needing for that plugin to potentially re-do fetching work). One such example is a plugin for gmod that when playing TTT and you are spectator, allowing you to hear everyone (disabling user setting positional audio, will have the last user set state saved and set back) and then when back alive, re-enabling positional audio.

Checks

• My commits follow the commit guidelines

[JordanPlayz158]

Will squash and conform to git/commit guidelines once the PR is deemed ready

Only not ideal thing currently which will either need to be changed or mentioned in the docs is each plugin that grabs the positional data via that function WILL be responsible for freeing the struct they made (which is implied) but ALSO before that, the positionalData->m_context and positionalData->m_identity fields, unless free is smart and notices the malloc'd memory and frees any fields inside the struct that were also malloc'd (which I would assume it wouldn't be aware of)

With some brief testing, the 2 strcpy's of context and identity may be redundant, if it is confirmed to be, that is great and I will push the code with the strcpy's removed

[davidebeatrici]

unless free is smart and notices the malloc'd memory and frees any fields inside the struct that were also malloc'd (which I would assume it wouldn't be aware of)

It doesn't, in fact the usual approach is to write a specific function that takes care of freeing the members before the struct itself. Something like this:

void free_struct(Struct *obj) {
	if (obj) {
		free(obj->var1);
		free(obj->var2);
	}

	free(obj);
}

With some brief testing, the 2 strcpy's of context and identity may be redundant, if it is confirmed to be, that is great and I will push the code with the strcpy's removed

I'm assuming you're referring to this piece of code:

char *context = posData.getContext().toUtf8().data();
positionalDataNoQt.m_context = (char *) malloc(strlen(context) + 1);
std::strcpy(positionalDataNoQt.m_context, context);

char *identity = posData.getPlayerIdentity().toUtf8().data();
positionalDataNoQt.m_identity = (char *) malloc(strlen(identity) + 1);
std::strcpy(positionalDataNoQt.m_identity, identity);

Copying the string's data is mandatory, otherwise your context and identity variables point to uninitialized data.

However, you can replace the strcpy() calls with memcpy() ones since you already know the length of the strings, which is better in terms of performance:

char *context = posData.getContext().toUtf8().data();
size_t size = strlen(context) + 1;
positionalDataNoQt.m_context = (char *) malloc(size);
std::memcpy(positionalDataNoQt.m_context, context, size);

char *identity = posData.getPlayerIdentity().toUtf8().data();
size = strlen(identity) + 1;
positionalDataNoQt.m_identity = (char *) malloc(size);
std::memcpy(positionalDataNoQt.m_identity, identity, size);

One more thing: C-style casts are allowed in C++, but it's recommended to use the C++-style ones instead:

positionalDataNoQt.m_context = static_cast< char * >(malloc(size));
positionalDataNoQt.m_identity = static_cast< char * >(malloc(size));

[JordanPlayz158]

I have done the suggestions from david except couldn't get the C++ destructor working but other than that, any other suggestions before making it no longer a draft (other than (probably) coming up with a better name for the struct than PositionalDataNoQt)?

[JordanPlayz158]

Did not know that marking ready for review removed it from the draft stage but I guess it is fine, I do think most of the things are ready.

[Krzmbrzl]

This needs a different name

[Krzmbrzl]

We don't need this.

[Krzmbrzl]

Suggested change

	PositionalDataNoQt **outPositionalData);
	PositionalDataNoQt *outPositionalData);

No need to allocate the passed struct on the heap (using e.g. new). The only thing required is that the plugin allocates some space for this struct (by creating an instance on the Stack) and then the Mumble side can simply populate the fields of that struct with the correct data.

[Krzmbrzl]

I would turn this into multiple functions: One for retrieving the positional data, one for retrieving the identity and one for retrieving the context. Only the latter two involve dynamic memory allocations due to the involvement of strings (for which you should use the existing string wrapper classes).

[Krzmbrzl]

Suggested change

	* Gets the positional audio data provided by OTHER plugins
	* Gets the currently used positional data

which plugin has supplied the data is left unspecified. It's not like this function won't work if the positional data has been supplied by the same plugin that now asks for it.

[JordanPlayz158]

Oh yeah true, your wording is better and conveys what I wanted to get across, made other in caps to convey it is not like the similarly named (imo it is similar names) fetchPositionalData (in my brain, get and fetch are similar, now get and put are more distinct imo but I see no reason to change the existing name of api functions when we can just be clear in the doc comments)

[Krzmbrzl]

For reference, the way I think the API should look like from the plugin's perspective is something like this:

DataStruct posData;
error_t retCode = api.getPositionalData(id, &posData);