padreEnvelopeProcessor: fixes/tweaks #1202

nofishonfriday · 2019-09-17T12:44:30Z

take LFO generator:

allow for generating more envelope points (use _GetEnvelopeStateChunkBig() instead of PadreGetEnvelopeState())
account for take playrate
fixes Bug: LFO Generator can't handle take envelopes with too much points:"Could not retrieve envelope object state!" #1158

padreUtils:

remove GetEnvelopeState()

+Snapshots:

TrackSends: use _GetEnvelopeStateChunkBig() (instead of GetEnvelopeStateChunk())

+add files: envelope.hpp/.cpp (place for contributor independent utility functions), update CMakeLists.txt

rationale for adding ~~UtilityFunctions.hpp/.cpp:~~ envelope.hpp/.cpp
Currently (general) utility functions are spread in various places in SWS:
sws_util / wol_Util / SnM_Util / XenUtils / ...

I thought I'd rather not add to this by adding my own NF_Util.
So in the past I've added functions to e.g. sws_util, but I don't consider it good style as these are not 'my' files.
UtilityFunctions.hpp/.cpp could serve as a place for collection of all future utility functions in one place (independent of contributor) and it also could be considered condensing some of the existing ones there.

Wasn't sure about a good function naming convention there.
I've underscored (to differ from Reaper API functions) but not prefixed (e.g. with NF_) because of a possible contributor independent collection (see above), contributor name prefix seems not obligatory imo.

Open to discussion/suggestions!

edit:
I see the builds failed. :(
Well, it does build here, need to investigate..

edit2:
https://travis-ci.com/reaper-oss/sws/jobs/236430802#L542
https://stackoverflow.com/questions/16861535/stdstring-has-no-member-named-front

Confused, I thought we're using C++11(+) now?
The other build errors I have solved I think.

cfillion · 2019-09-17T18:56:52Z

error: no member named 'front' in 'std::basic_string<char>'
Confused, I thought we're using C++11(+) now?

Ah, this is because SWS is built targeting macOS 10.5. We can use C++11 syntax and features now but the standard library is ultimately limited to what's available in macOS 10.5's libstdc++. Apple started to add support for C++11 in 10.7's ~~libstdc++~~ libc++.

sws/.travis.yml

Line 14 in 629a566

-DCMAKE_OSX_DEPLOYMENT_TARGET=10.5

nofishonfriday · 2019-09-17T22:04:35Z

Reading Directly write into char* buffer of std::string:

so using &str[0] instead of &str.front() would be ok in this case as we have C++11 'environment'?
edit:
Hm..if current standard library we're using doesn't comply to C++11 standard (basic_string object shall be stored contiguously) then probably not I think.

cfillion · 2019-09-18T01:16:22Z

Hm..if current standard library we're using doesn't comply to C++11 standard (basic_string object shall be stored contiguously) then probably not I think.

Right. The standard didn't guarantee it, but I'm pretty sure the pre-C++11 libstdc++ does (though it uses copy-on-write, which shouldn't be an issue here).

operator[0], data and c_str all give the same addresses (tested on 10.6 and 10.10), implying they're all contiguous and null-terminated (since c_str always has to be).

EDIT: Double-checked from macOS 10.5's libstdc++'s source code (basic_string.h & basic_string.tcc).

(Alternatively we could bump the min. macOS version to 10.7 and switch to libc++, use a vector<char> or even a WDL_FastString...)

nofishonfriday · 2019-09-18T18:01:11Z

Thanks!
Also found this which pretty much comes to the same conclusion:
https://stackoverflow.com/a/760900

Phew, turned out more complicated (for me) than expected, but it finally builds now at least. 😄
(so helpful being able to see the errors with the auto build system)
.If you'd get around to code review I'd appreciate.

cfillion · 2019-10-04T11:41:56Z

ObjectState/TrackSends.cpp

+					catch (_bad_get_env_chunk_big) {
+
+						DisplayStoreSendEnvError(tr, "AUXVOL");
+						AUXVOLstr = "";


AUXVOLstr is already initialized to an empty string by its constructor. There is no need re-assign it a new empty value.

cfillion · 2019-10-04T11:46:06Z

ObjectState/TrackSends.cpp

@@ -432,3 +454,25 @@ void TrackSends::GetChunk(WDL_FastString* chunk)
 	for (int i = 0; i < m_sends.GetSize(); i++)
 		m_sends.Get(i)->GetChunk(chunk);
 }
+
+void TrackSends::DisplayStoreSendEnvError(MediaTrack* tr, const char* envName)


Instead of taking a const char* and doing string comparison with strncmp, this could take an Enum (integer comparison).

Also, maybe this could take the exception (const bad_get_env_chunk_big &ex) and display its message (ex.what()) to further explain what the error is about (100 MiB buffer limit reached vs memory allocation failed).

cfillion · 2019-10-04T11:49:02Z

ObjectState/TrackSends.cpp

+		msg.append(__LOCALIZE("Error storing send mute envelope!", "sws_mbox"));
+	}
+
+	MessageBox(g_hwndParent, msg.c_str(), __LOCALIZE("SWS Snapshots - Error", "sws_mbox"), MB_OK);


The message can be built using only the one stringstream.

std::ostringstream ss; ss << "Track " << static_cast<int>(GetMediaTrackInfo_Value(tr, "IP_TRACKNUMBER")) << ":\n"; switch(envType) { case AUXVOL: ss << __LOCALIZE("Error storing send volume envelope!", "sws_mbox"); break; case AUXPAN: ss << __LOCALIZE("Error storing send pan envelope!", "sws_mbox"); break; // ... } MessageBox(..., ss.str().c_str(), ...);

cfillion · 2019-10-04T11:50:37Z

Padre/padreEnvelopeProcessor.cpp

+	// account for take playrate, #1158
+	if (take)
+	{
+		double playrate = GetMediaItemTakeInfo_Value(take, "D_PLAYRATE");
+		dFreq = dFreq / playrate;
+	}
+


This appears to be unrelated to the buffer size fix, and should probably be split in a separate commit.

EDIT: Could be written as:

if(take) // account for take playrate, #1158 dFreq /= GetMediaItemTakeInfo_Value(take, "D_PLAYRATE");

cfillion · 2019-10-04T11:56:55Z

Utility/UtilityFunctions.cpp

+// throws _bad_get_env_chunk_big on failure
+// until maybe it gets enhanced one day
+// https://forum.cockos.com/showthread.php?p=2142245#post2142245
+std::string _GetEnvelopeStateChunkBig(TrackEnvelope* envelope, bool isUndo /*= false*/)


In my opinion this should just be called GetEnvelopeStateChunkBig. The leading underscore makes it look like it's some kind of private/special/internal use/secret function that shouldn't be used outside of its compilation unit.

Maybe namespacing would be a better option here if there is a risk of name conflict.

cfillion · 2019-10-04T11:59:51Z

Utility/UtilityFunctions.cpp

+	_bad_get_env_chunk_big exExceeding100MiB("_GetEnvelopeStateChunkBig() exception (>  100 MiB)");
+	_bad_get_env_chunk_big exBad_alloc("_GetEnvelopeStateChunkBig() exception (bad_alloc)");


There is no need to construct the exception object(s) ahead of time. This would do job just as well (avoiding doing more work unless an exception is actually being thrown–which is already a slow process):

throw bad_get_env_chunk_big("The envelope chunk size exceeded the 100 MiB limit");

cfillion · 2019-10-04T12:03:13Z

Utility/UtilityFunctions.cpp

+
+		if (buffer.size() > 100 << 20) { // 100 MiB
+			throw exExceeding100MiB;
+			std::cout << "_GetEnvelopeStateChunkBig() exception (>  100 MiB)";


This std::cout (and the one below) will never be reached (dead code).

cfillion · 2019-10-04T12:05:05Z

Utility/UtilityFunctions.hpp

+private:
+	std::string msg;
+public:
+	explicit _bad_get_env_chunk_big(const std::string& message) {};


The message is lost here, and what() will always return an empty string. You could make use of std::runtime_error (instead of essentially duplicating what it does):

class bad_get_env_chunk_big : public std::runtime_error { public: using runtime_error::runtime_error; };

https://en.cppreference.com/w/cpp/error/runtime_error

cfillion · 2019-10-04T12:32:16Z

ObjectState/TrackSends.cpp

@@ -29,6 +29,8 @@
 #include "../reaper/localize.h"
 #include "../SnM/SnM_Dlg.h"
 #include "TrackSends.h"
+#include "Utility/UtilityFunctions.hpp"
+#include <sstream>


sstream is already included in stdafx.h, the precompiled header (it's only precompiled on Windows).

cfillion

UtilityFunctions.hpp/.cpp could serve as a place for collection of all future utility functions in one place (independent of contributor) and it also could be considered condensing some of the existing ones there.

👍 for contributor-independent functions and classes, but I find "UtilityFunctions" rather vague and catch-all. How about a new envelope.cpp or similar to be a bit more structured?

nofishonfriday · 2019-10-04T22:22:52Z

Thanks for the comprehensive review!

class bad_get_env_chunk_big : public std::runtime_error {
public:
  using runtime_error::runtime_error;
};

Hm..fine on Win/Linux, no luck on Mac:
https://travis-ci.com/reaper-oss/sws/jobs/242268219#L523
(I'm guessing no runtime_error in 10.5 standard library?)

Any alternative suggestion?

cfillion · 2019-10-04T22:37:26Z

Adding #include <stdexcept> should do the trick.

nofishonfriday · 2019-10-04T22:45:48Z

Thanks.

Also, maybe this could take the exception (const bad_get_env_chunk_big &ex) and display its message (ex.what()) to further explain what the error is about (100 MiB buffer limit reached vs memory allocation failed).

I had some difficulties with this:

bad_get_env_chunk_big is imported in padreEnvelopeProcessor.cpp, so I couldn't use it in the function ~~definition~~ declaration in padreEnvelopeProcessor.h

Now takes the exception string instead

__LOCALIZE(exception.c_str(), "sws_mbox"); doesn't seem to work (I wonder why, as it does seem to take a const char[]?)

Used __localizeFunc() instead, is this ok?
https://github.com/reaper-oss/sws/pull/1202/files#diff-cd23b1cfff5934876e132daa9092852bR471

The rest is done according to your review (I think/hope).

edit:
I put envelope.hpp now in stdafx.h as it seems a good candidate for pre-compiling?

cfillion · 2019-10-04T23:05:10Z

__LOCALIZE(exception.c_str(), "sws_mbox"); doesn't seem to work (I wonder why, as it does seem to take a const char[]?)

I'll check tomorrow but I'm pretty sure the langpack generation tool only supports string literals. Alternatively __LOCALIZE could be used when the exception is being thrown.

EDIT: Confirmed. BuildUtils/BuildLangPack.cpp and https://github.com/reaper-oss/sws/wiki/How-to-write-localizable-code%3F.

bad_get_env_chunk_big is imported in padreEnvelopeProcessor.cpp, so I couldn't use it in the function definition in padreEnvelopeProcessor.h

bad_get_env_chunk_big can be used as the type of a function parameter in TrackSends.h without including envelope.hpp by forward-declaring it:

namespace envUtil {
  class bad_get_env_chunk_big;
};

void DisplayStoreSendEnvError(MediaTrack* tr, EnvTypes envName, const envUtil::bad_get_env_chunk_big &);

However, because DisplayStoreSendEnvError does not use any members of the TrackSends class and is not used outside of TrackSends.cpp, it does not need to be put in the header. It could be a static function living only within the scope of the .cpp.

static void DisplayStoreSendEnvError(MediaTrack* tr, EnvTypes envType, const envUtil::bad_get_env_chunk_big &ex)
{
}

// code that uses it below (or add a prototype for the function before said code)

nofishonfriday · 2019-10-04T23:34:28Z

However, because DisplayStoreSendEnvError does not use any members of the TrackSends class and is not used outside of the TrackSends.cpp, it does not need to be put in the header at all. It could be a static function living only within the scope of the .cpp.

The only thought to declare it in the header was avoiding using forward declarations / having to put the function somewhere in the middle (both I found a bit cluttery) ;D
But I see your point, will do. :)

nofishonfriday · 2019-10-05T13:30:01Z

It could be a static function living only within the scope of the .cpp.

Done.
Also moved enum EnvTypes from TrackSends.h to TrackSends.cpp (making it strongly typed enum, default to int, C++11).

Alternatively __LOCALIZE could be used when the exception is being thrown.

Done.

Also changed namespace envUtil to namespace envelope (actually I've not yet seen a camel-cased namespace :D)
Maybe could even be namespace env for less typing (similar to std::)?

Btw. why did you suggest envelope.hpp/cpp - lowercase?
Somehow I have in mind filenames always upper case, any rule you follow here?

Utility/envelope.cpp

cfillion

Btw. why did you suggest envelope.hpp/cpp - lowercase?
Somehow I have in mind filenames always upper case, any rule you follow here?

Force of habit–I rarely deal with camel cased filenames for source files outside of SWS. I should probably have suggested Envelope.cpp for consistency as most source files in this repository use upper camel case (except those that don't 😄 ).

nofishonfriday · 2019-10-05T18:18:54Z

done #1202 (review)

Utility/envelope.cpp

take LFO generator: - allow for generating more envelope points (use envelope::GetEnvelopeStateChunkBig() instead of PadreGetEnvelopeState()) reaper-oss#1158 (partly) padreUtils: - remove GetEnvelopeState() +add files: envelope.hpp/.cpp (place for general envelope utility functions), #include in stdafx.h, update CMakeLists.txt +Snapshots: - TrackSends: use envelope::GetEnvelopeStateChunkBig() (instead of GetEnvelopeStateChunk())

…reaper-oss#1158

…github.com/nofishonfriday/sws into sws-2019-09, PR reaper-oss#1202 * 'fix-padre-lfo+get-envelope-state-chunk-big' of https://github.com/nofishonfriday/sws: padreEnvelopeProcessor: take LFO generator: account for take playrate, reaper-oss#1158 padreEnvelopeProcessor: fixes/tweaks

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch 3 times, most recently from ef957fe to 6f6303f Compare September 18, 2019 16:06

cfillion reviewed Oct 4, 2019

View reviewed changes

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch from 6f6303f to 1460bdf Compare October 4, 2019 21:29

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch from 1460bdf to 05e6486 Compare October 4, 2019 22:43

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch from 05e6486 to 5f82324 Compare October 5, 2019 13:19

cfillion reviewed Oct 5, 2019

View reviewed changes

Utility/envelope.cpp Outdated Show resolved Hide resolved

cfillion reviewed Oct 5, 2019

View reviewed changes

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch from 5f82324 to 0246a6c Compare October 5, 2019 18:17

cfillion reviewed Oct 5, 2019

View reviewed changes

Utility/envelope.cpp Outdated Show resolved Hide resolved

nofishonfriday force-pushed the fix-padre-lfo+get-envelope-state-chunk-big branch from 0246a6c to db89dad Compare October 6, 2019 11:34

padreEnvelopeProcessor: take LFO generator: account for take playrate, …

50181bf

…reaper-oss#1158

nofishonfriday mentioned this pull request Oct 6, 2019

sws-2019-09 #1195

Merged

cfillion merged commit 50181bf into reaper-oss:next Oct 14, 2019

nofishonfriday deleted the fix-padre-lfo+get-envelope-state-chunk-big branch October 16, 2019 12:30

nofishonfriday mentioned this pull request Jan 17, 2020

Add regex support in Auto Color for track, marker, region name filters #1291

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

padreEnvelopeProcessor: fixes/tweaks #1202

padreEnvelopeProcessor: fixes/tweaks #1202

nofishonfriday commented Sep 17, 2019 •

edited

Loading

cfillion commented Sep 17, 2019 •

edited

Loading

nofishonfriday commented Sep 17, 2019 •

edited

Loading

cfillion commented Sep 18, 2019 •

edited

Loading

nofishonfriday commented Sep 18, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion left a comment

nofishonfriday commented Oct 4, 2019 •

edited

Loading

cfillion commented Oct 4, 2019 •

edited

Loading

nofishonfriday commented Oct 4, 2019 •

edited

Loading

cfillion commented Oct 4, 2019 •

edited

Loading

nofishonfriday commented Oct 4, 2019

nofishonfriday commented Oct 5, 2019 •

edited

Loading

cfillion left a comment •

edited

Loading

nofishonfriday commented Oct 5, 2019

		_bad_get_env_chunk_big exExceeding100MiB("_GetEnvelopeStateChunkBig() exception (> 100 MiB)");
		_bad_get_env_chunk_big exBad_alloc("_GetEnvelopeStateChunkBig() exception (bad_alloc)");

padreEnvelopeProcessor: fixes/tweaks #1202

padreEnvelopeProcessor: fixes/tweaks #1202

Conversation

nofishonfriday commented Sep 17, 2019 • edited Loading

cfillion commented Sep 17, 2019 • edited Loading

nofishonfriday commented Sep 17, 2019 • edited Loading

cfillion commented Sep 18, 2019 • edited Loading

nofishonfriday commented Sep 18, 2019 • edited Loading

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion Oct 4, 2019 • edited Loading

Choose a reason for hiding this comment

cfillion left a comment

Choose a reason for hiding this comment

nofishonfriday commented Oct 4, 2019 • edited Loading

cfillion commented Oct 4, 2019 • edited Loading

nofishonfriday commented Oct 4, 2019 • edited Loading

cfillion commented Oct 4, 2019 • edited Loading

nofishonfriday commented Oct 4, 2019

nofishonfriday commented Oct 5, 2019 • edited Loading

cfillion left a comment • edited Loading

Choose a reason for hiding this comment

nofishonfriday commented Oct 5, 2019

nofishonfriday commented Sep 17, 2019 •

edited

Loading

cfillion commented Sep 17, 2019 •

edited

Loading

nofishonfriday commented Sep 17, 2019 •

edited

Loading

cfillion commented Sep 18, 2019 •

edited

Loading

nofishonfriday commented Sep 18, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

cfillion Oct 4, 2019 •

edited

Loading

nofishonfriday commented Oct 4, 2019 •

edited

Loading

cfillion commented Oct 4, 2019 •

edited

Loading

nofishonfriday commented Oct 4, 2019 •

edited

Loading

cfillion commented Oct 4, 2019 •

edited

Loading

nofishonfriday commented Oct 5, 2019 •

edited

Loading

cfillion left a comment •

edited

Loading