Fix easter egg and refactor MilkdropNoise class

[kblaschke]

Two small things I've fixed/reworked for the 4.1 release:

• Fixed an issue with the sigma value (AKA "easter egg parameter") being zero. It'll be used only when set to values >= 0.001 now, otherwise projecTM will just revert to the default sigma of 1.0. Updated the initial value and the API function documentation accordingly.
• Refactored the MilkdropNoise class so it's longer required to create an instance. It was only used once on startup anyway, and the noise functions were already static. Also moved the texture creation into MilkdropNoise, which now returns a ready-to-use shared pointer to a Texture instance.

[dpw13]

It might make more sense to say something like m_easterEgg = max(m_easterEgg, 0.001). Having it snap to 1.0 only if it gets very small seems unexpected to me.

[kblaschke]

If it's always properly initialized, then I agree.

Setting it to 1 here will make sure the proper default value is used if someone uses 0 as the default value in an external app integration and passes it in after construction, e.g. in existing apps made by someone who didn't know about this limitation. Besides that, this "easter egg" isn't really noticeable in any case.

[dpw13]

Another option would be to just return m_presetDuration if m_easterEgg is zero, since that's what the normal distribution would theoretically do with stddev = 0. No action needed though since there are bigger fish to fry.

[dpw13]

Looks good overall. I'll leave it up to you to decide whether you want to remove the extra random data copy or not.

[dpw13]

Any particular reason this is in a separate block?

[kblaschke]

Slightly reduces the lifetime of the textureData vector, which is only required until after the glTexImageXD call. Not really a concern though as the method exits shortly after, if you find it less readable this way I can just remove the blocks.

[dpw13]

Not really a big deal either way, just making sure it was intentional. It's consistent with other places in the code so I say leave it.

[dpw13]

This is redundant, you already assign dst above.

[kblaschke]

No, it's not.
If you look closely, you'll notice that dstis being incremented in the first loop. This assignment resets it to the start of the buffer. Without this line, projectM will just crash.

[dpw13]

Aha, I missed the increment. You're right, my bad!

[dpw13]

This is performing an additional (unnecessary) copy of textureData. I suggest either allocating textureData into a smart pointer and returning that or passing in the std::vector by reference instead to avoid the copy.

[kblaschke]

I'm well over 99% sure most modern compilers will always use copy elision here, as it's a rather simple case. So using smart pointers or a reference would increase code complexity, but not provide any advantages.

At least on my machines, I've checked this and .data() returns the same memory address inside the function (after resize()) and in the calling function as well in both release and debug builds, so no data has been copied to another memory location.

[dpw13]

I'm glad the compiler optimizes this away, but it's still allocating a vector on the stack, then returning that vector through a function call, which according to the spec will call the copy constructor. If you don't want to fix it that's fine (this is done during preset initialization so it doesn't really affect runtime performance) but it's not best practice.

[kblaschke]

If you'd check this, you'll see that the compiler turns it into a move operation, and not even that in the std::move sense - the compiler will simply make the variable it's being assigned to point to the stack object. Even if the vector itself would be copied (which is perfectly allowed as you said), it's just a heap pointer, the alloc size and the element count, so 24 bytes to copy on 64 bit systems. Not that much of a hassle.

[dpw13]

This allocates a new texture each time this is called. That's fine if each of these is only called once by TextureManager, but if it's called twice it'll generate a new texture with different values. No action needed, just a note. In theory you could initialize a static field in this class and return it if it's been constructed, but I don't think the change is worth it since this should only be called once.

[kblaschke]

I don't understand the issue at hand here.
The noise is pseudo-random, so for each call I'd actually expect it to be different (or, rather even require it). Plus, this function is actually there to create a new texture upon each call (not reuse any existing one), because users might want different noise textures for each preset (currently, these are just generated once per projectM instance).

[dpw13]

The previous implementation defined texture data and initialized it, and that texture data is referenced directly. The new implementation creates texture data each time it's called. It doesn't matter as long as this isn't called multiple times for a single preset (which it isn't) so it doesn't matter. No changes required.

[kblaschke]

The series of events was changed, but technically, it does the exact same thing as before. Just to compare, here's how it worked previously each time TextureManager::Preload() was called:

1. Create an instance of MilkdropNoise.
2. The MilkdropNoise constructor called each static noise generator function once and stored the resulting texture data as a raw pointer
3. For each noise texture image, a GL texture was then created and the image data copied into it (via glTexture2D/glTexture3D).
4. A projectM Texture instance was then created with the resulting GL texture ID for use in presets, stored as a shared pointer.
5. The MilkdropNoise instance was deleted at the end of the Preload function, including the image data.

Now the MilkdropNoise class is purely static, and texture creation was just moved into each noise function, like this:

1. For each noise texture, the static generator function in Milkdrop noise is called, which then does:
   1. Generate the pixel data for the requested noise image.
   2. Create a GL texture and upload the image data into it.
   3. Delete the local pixel data.
   4. Create a Texture instance with the just-created texture ID and return it as a shared pointer.
2. In Preload(), the returned shared_ptr instance is then added to the texture storage.

[dpw13]

Another option would be to just return m_presetDuration if m_easterEgg is zero, since that's what the normal distribution would theoretically do with stddev = 0. No action needed though since there are bigger fish to fry.