jamoma / JamomaCore

Jamoma Frameworks for Audio and Control Structure
Other
36 stars 14 forks source link

TTSoundfile::setFilePath should b eoverloaded to accept TTString #189

Closed nwolek closed 10 years ago

nwolek commented 10 years ago

Currently takes a TTValue. This leads to situations were the code is casting a TTString to a TTValue, which the method then casts back to a TTString.

Noticed this while working on issue #175. Making a new issue so I won't forget to address this later.

tap commented 10 years ago

​Is this really a problem? If so, how?

Calling setFilePath is loading a soundfile, right? If so then this machinery should be inconsequential if it were to be benchmarked? Has it been benchmarked and shown otherwise?

Overloading something that is exposed as an attribute for a TTObject is problematic because when you send a message or call setAttribute then the resolution from a symbolic name to the implemented method becomes ambiguous -- resulting in unpredictable behavior at runtime.

best, | : ||

On Wed, Dec 4, 2013 at 8:05 AM, nwolek notifications@github.com wrote:

Currently takes a TTValue. This leads to situations were the code is casting a TTString to a TTValue, which the method then casts back to a TTString.

Noticed this while working on issue #175https://github.com/jamoma/JamomaCore/issues/175. Making a new issue so I won't forget to address this later.

— Reply to this email directly or view it on GitHubhttps://github.com/jamoma/JamomaCore/issues/189 .

nwolek commented 10 years ago

Sorry, I thought I was making a note about an internal method. On closer inspection, I see now that it is an exposed attribute of the TTSoundfile object. I get what you are saying about staying consistent for attributes.

Just thought that it was goofy in my test routine that I created a TTString, then wrapped it in then TT macro to turn it into a TTValue, only to have the setFilePath method convert it back to a TTString.

For an internal method, that would seem to be unnecessary overhead. But for the consistency of attributes, it make total sense.

Cheers.

tap commented 10 years ago

Thanks, Nathan. There might be a real issue in here (or not)!

You say "wrapped in a TT macro" -- that makes it not a TTString but in fact a TTSymbol. Do we need to do some work to make sure that we can pass TTStrings more directly through the chain without doing symbol lookups?

best, | : ||

On Wed, Dec 4, 2013 at 9:19 AM, nwolek notifications@github.com wrote:

Sorry, I thought I was making a note about an internal method. On closer inspection, I see now that it is an exposed attribute of the TTSoundfile object. I get what you are saying about staying consistent for attributes.

Just thought that it was goofy in my test routine that I created a TTString, then wrapped it in then TT macro to turn it into a TTValue, only to have the setFilePath method convert it back to a TTString.

For an internal method, that would seem to be unnecessary overhead. But for the consistency of attributes, it make total sense.

Cheers.

— Reply to this email directly or view it on GitHubhttps://github.com/jamoma/JamomaCore/issues/189#issuecomment-29812973 .

nwolek commented 10 years ago

The method is setFilePath() here: https://github.com/jamoma/JamomaCore/blob/master/DSP/extensions/SoundfileLib/sources/TTSoundfile.cpp#L68

You'll see that it take a TTValue as input. When I do the following, it does NOT work:

TTString testSoundPath = "/the/path/that/use.aiff"
this->setFilePath(testSoundPath);

But the following DOES work:

TTString testSoundPath = "/the/path/that/use.aiff"
this->setFilePath(TT(testSoundPath));

Does this clarify what I am talking about more?

tap commented 10 years ago

Makes sense -- I added a new ticket for that. Thanks!

best, | : ||

On Wed, Dec 4, 2013 at 12:50 PM, nwolek notifications@github.com wrote:

The method is setFilePath() here:

https://github.com/jamoma/JamomaCore/blob/master/DSP/extensions/SoundfileLib/sources/TTSoundfile.cpp#L68

You'll see that it take a TTValue as input. When I do the following, it does NOT work:

TTString testSoundPath = "/the/path/that/use.aiff" this->setFilePath(testSoundPath);

But the following DOES work:

TTString testSoundPath = "/the/path/that/use.aiff" this->setFilePath(TT(testSoundPath));

Does this clarify what I am talking about more?

— Reply to this email directly or view it on GitHubhttps://github.com/jamoma/JamomaCore/issues/189#issuecomment-29833094 .