-
Notifications
You must be signed in to change notification settings - Fork 3
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
227: Save (and restore) number of sources and first source ID in presets #229
Conversation
…t the first time it's loaded. Or maybe it's just not saved properly?
Source/cg_PresetsManager.cpp
Outdated
for (auto & source : mSources) { | ||
SourceSnapshot snapshot{}; | ||
SourceSnapshot snapshot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
est-ce qu'on est surs que ça change rien ? ça peut avoir un impact si le type est "plain-old-data".
ex.: https://gcc.godbolt.org/z/nzqbPvvv8
ici on voit que dans le premier cas en haut, la mémoire est renvoyée directement de la fonction; dans le second cas elle se fait zeroed avant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh wow ok, bien vu!
Source/cg_PresetsManager.cpp
Outdated
terminalPosition | ||
= juce::Point<float>{ inversedTerminalPosition.getX(), inversedTerminalPosition.getY() * -1.0f }; | ||
auto const inversedTerminalPosition{ inversedNormalizedTerminalPosition * 2.0f - juce::Point<float>{ 1.0f, 1.0f } }; | ||
terminalPosition = juce::Point<float>{ inversedTerminalPosition.getX(), inversedTerminalPosition.getY() * -1.0f }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
j'ai l'impression qu'il y a pas mal de petits changements de formatting qui inviteraient à faire une MR globale de clang-format, qu'en penses-tu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(à noter qu'il faut vraiment la faire à un moment ou tout est mergé et il n'y a rien d'autre en cours sinon ça va être l'enfer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha ouais 💯
Source/cg_PresetsManager.cpp
Outdated
auto const xName{ getFixedPosSourceName(FixedPositionType::terminal, SourceIndex{ 0 }, 0) }; | ||
auto const yName{ getFixedPosSourceName(FixedPositionType::terminal, SourceIndex{ 0 }, 1) }; | ||
auto const zName{ getFixedPosSourceName(FixedPositionType::terminal, SourceIndex{ 0 }, 2) }; | ||
//TODO VB: is terminal the same as final? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pas sûr qu'on veuille garder ces commentaires dans la version finale ^^'
Source/cg_PresetsManager.cpp
Outdated
|
||
// For some legacy reason, we store a normalized value with inversed Y and elevation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kinda makes sense, a lot of spatialization systems work like this
Source/cg_SourceSnapshot.hpp
Outdated
@@ -56,6 +56,15 @@ struct SourcesSnapshots { | |||
SourceSnapshot & operator[](SourceIndex const index); | |||
int size() const { return secondaries.size() + 1; } | |||
|
|||
juce::String toString () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this method can be marked const
juce::String toString () | ||
{ | ||
juce::String ret; | ||
ret += primary.position.toString() + ", " + primary.z.toString() + "; "; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hopefully one day juce supports std::format :D with it it would be:
ret += std::format("{}, {}; ", primary.position, primary.z);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right... another thing is we'd need to change the c++ version in the projucer. I'll make a note of it
J'ai testé avec les fix, ça marche ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maintenant que j'ai les pouvoirs, j'approuve !
Not saving source-link for now because the manual explains we're not saving those since they are automated