Skip to content
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

simplified AlignedBuf, corrected dexedTheme, code purification related to MidiMonitor #434

Closed

Conversation

FulopNandor
Copy link
Contributor

Dear Pascal,
Please, accept my current pull request, which contains modifications related to the following three area:

  1. Definition of AlignedBuf class being in aligned_buf.h is simplified utilizing the aligment mechanisms provided by the modern C++ compilers. (The value of 8 is applied as a default alignment, I guess, it is appropriate for both 64-bit and 32-bit CPU systems.)

  2. A symbol IMPLEMENT_MidiMonitor in SysexComm.h is introduced to allow compilation/skipping of items related to MidiMonitor class. The definition of the class itself was commented out in the GlobalEditor.cpp file, however some related variables and code snippets located in some other files were not (and it was not clear whether certain commented code snippets were originally intended only for operating the MidiMonitor).
    Now all the code snippets identified are enclosed by the proper #ifdef-#endif structures controlled by the IMPLEMENT_MidiMonitor.
    Furthermore, the code of the MidiMonitor class has been updated, but only in a way that allows it to be compiled and linked without errors to achieve a functional state.
    MidiMonitor-implemented
    However, as its usage is still not recommended, the IMPLEMENT_MidiMonitor
    is undefined in the sources of this commit.

  3. DXLookNFeel.cpp is corrected to overcome the issue DexedTheme.xml Colors are Inconsistent + Background Glitch + Documentation Typo #113.
    Furthermore, the content of file ~/Documentation/DexedTheme.md is updated providing a working example to demonstrate how to
    a) change of colors of items of a popup menu; the current code produces erratic translucent popup menu:
    LookNFeel-bad-translucent-popup-menu
    while the correction in this commit displays properly colored submenu, so maybe Issue No #113 could be closed now:
    LookNFeel-colored-popup-menu
    b) replace the current red color LEDs with a custom green color LED on the UI of Dexed (an example file GreenLight_28x28.png is supplied) via DexedTheme.xml:
    LookNFeel-green-LEDs
    c) Furthermore, the ScrollBar::thumbColourId is also registered to allow modify its colors via DexedTheme.xml. I guess, setting the color of thumb to a lighter one (e.g. lightgray) might result a more comfortable view of the cartridge (.SYX patch) selector window for those cases, when large folders are opened.
    In the current state the color of thumbnail can be seen here:
    LookNFeel-default-thumb-in-cartridge-selector
    and its lightgray version is shown here:
    LookNFeel-lightgray-thumb-in-cartridge-selector

@FulopNandor FulopNandor force-pushed the 3_alignedbuf_dexedtheme branch from 08dae46 to a9b9277 Compare July 16, 2024 15:05
@asb2m10
Copy link
Owner

asb2m10 commented Jul 24, 2024

Hi @FulopNandor,

I still have to understand the need to provide a smaller AlignedBuf, does clang is included with this ? what about older compilers ? Maybe I'm wrong but I don't see any benefits to target code on specific compilers.

That said your code on Dexed theme and MidiMonitor would have be merge if the AlignedBuf modification would not be included. Sometime a small PR per feature are easier to deal with.

Thanks.

@FulopNandor
Copy link
Contributor Author

Dear Pascal,

Thank you for your comments. I absolutely understand your concerns and I agree that smaller PRs per feature are easier to manage. Therefore, I will split this PR into smaller ones.

As for your question about the AlignedBuf: primarily, my main goal was to reduce the access time of the elements within the AlignedBuf objects, rather than reducing their size.. Whenever an element of AlignedBuf is written/read out, that is done by a call to

  T *get() {
    return (T *)((((intptr_t)storage_) + alignment - 1) & -alignment);
  }

in lines 29-31 being in dexed/Source/msfa/aligned_buf.h file to guarantee that the alignment of the first element falls on a 16-byte offset boundary. I guessed only, that this mechanism] above, providing the 16-byte boundary might have advantage of mobile applications (maybe it was inherited from the original MSFA code?), when the proper usage of SIMD instructions require 16-byte boundaries. However, if we build Dexed for other 64-bit or 32-bit CPU platforms via C++ compilers but without SIMD support, we may reduce both the access time and size of AlignedBuf, if we simply use the alignas(8) provided by the C++ compilers and simple indexing of its internal array storage_.
Even, the following comment in the code
// Note that if we were on C++11, we'd use aligned_storage or somesuch.
also motivated me to modify the definition of AlignedBuf. (I have made already some very simple benchmark tests to compare the performance of the original AlignedBuf and its alignas()-ed versions, I will enclose these result in the standalone PR of the new simplified_AlignedBuf.)
And, related to the compatibility with alignment mechanisms for Clang and older compilers, I can tell you that based on my readings from various articles and forum discussions, and they should work with Clang and most other modern C++ compilers that support C++11 or later. However, I must admit that I have not personally tested these claims. My experience is limited to the most recent versions of the MSC and GCC compilers. (i.e. I guessed only, that most C++ compilers, released after about 2013, should accept the format alignas(8) to provide the memory alignment of the variables).
Briefly: I am going to strive to find a more definitive answer and solution regarding the compatibility of this attribute with Clang and older compilers and will try to update the code accordingly - and enclosing some proofs to demonstrate the better performance .

@FulopNandor
Copy link
Contributor Author

I am going to split this PR into three smaller parts, so I close this one.

@FulopNandor FulopNandor deleted the 3_alignedbuf_dexedtheme branch July 27, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants