Skip to content

Conversation

@TheAssassin
Copy link
Member

This PR attempts to rewrite most of the sounds management code. It provides a new template class soundslot_collection, which supports both a map and vector based storage implementation, to efficiently manage map and game sounds. Also, game sounds are now registered in the C++ code so that their registration does no longer rely on matching the order of the registration calls in CubeScript with the enum in the code.

@TheAssassin TheAssassin added enhancement New feature or request refactoring labels Aug 1, 2020
@TheAssassin TheAssassin marked this pull request as draft August 1, 2020 22:03
This commit fixes compatibility with STL collection classes (e.g., std::map). The default generated one doesn't work very well, as it doesn't copy the string properly.
@robalni
Copy link
Contributor

robalni commented Aug 2, 2020

Why do you use a map for gamesounds? I know there is a comment that says that it makes sense to do that but why? If the key is an int that comes from an enum then it feels to me like it should make more sense to use a vector.

@TheAssassin
Copy link
Member Author

TheAssassin commented Aug 2, 2020

We don't fill every slot with a sound, there are many gaps, so using a vector would either require implementing the null object pattern properly, or using pointers to be able to use nullptr to represent gaps. I've first attempted to use a vector at first as well, but I experienced memory issues, and IMO the obvious approach is to use a map. We could optimize on that later on. Performance wise, you don't notice any differences.

Edit: of course, there is std::optional, but a null object is probably the more idiomatic approach.

Should probably be removed entirely, as the samples are cached already and recreating the struct instance is not very expensive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants