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

Are strings needed? #19

Open
Murf opened this issue Mar 4, 2023 · 15 comments
Open

Are strings needed? #19

Murf opened this issue Mar 4, 2023 · 15 comments

Comments

@Murf
Copy link

Murf commented Mar 4, 2023

Hi, I am trying to use this header library for Reason and it does not cater for strings.
Is the use of strings crucial to your implementation or just a nicety?

@Murf
Copy link
Author

Murf commented Mar 4, 2023

I have replaced std::string with char* and am getting "NaN" returned from processSample.
Reason doesnt allow alloc etc, so I am not sure how these arrays would be defined.
I tried "char[20]" and I get this error:
./wdf/chowdsp_wdf/wdf/wdf_base.h:115:13: error: reference to overloaded function could not be resolved; did you mean to call it?
next.connectToNode (this);
^~~~

@jatinchowdhury18
Copy link
Contributor

Hello!

In general, strings are not required for this library, but they are quite handy when working with the polymorphic API. That said, maybe we could add a compile option to optionally remove the string dependency?

In your case, I might suggest explicitly using the templated API, by including chowdsp_wdf/wdft/wdft.h, which will avoid the including the <string> header. The templated API also provides better performance, but is limited to cases where the circuit topology is known at run-time.

If you're getting NaN back from your process sample method, the most likely cause is that there is some circuit element in your WDF tree with an impedance of zero.

@Murf
Copy link
Author

Murf commented Mar 4, 2023

Thanks for getting back to me!
Yes this is a compile time environment so the templated version would be preferable.
However.. in your latest release there is no wdft folder, I can only find it in your older repo before it was consolidated.
Murf.

@Murf
Copy link
Author

Murf commented Mar 4, 2023

Belay that, here it is :)
/chowdsp_wdf/include/chowdsp_wdf/wdft/wdft.h

@Murf
Copy link
Author

Murf commented Mar 4, 2023

An now this error? Please advise!
In file included from ./wdf/chowdsp_wdf/wdft/wdft.h:15:
./wdf/chowdsp_wdf/wdft/wdft_adaptors.h:224:5: error: unknown type name 'CHOWDSP_WDF_MAYBE_UNUSED'
CHOWDSP_WDF_MAYBE_UNUSED WDFParallelT<T, P1Type, P2Type> makeParallel (P1Type& p1, P2Type& p2)
^

@Murf
Copy link
Author

Murf commented Mar 4, 2023

Ok I see that it is wdf_base.h that has the string definition, and it is including #include "../wdft/wdft.h" already.
From your advise I assumed I would be not using the header files with string declarations?

@jatinchowdhury18
Copy link
Contributor

Oh yeah, I guess there's a couple of global definitions in the main header file, that would be missing if you just include one of the internals. In that case, probably the simplest thing to do will be to copy the main header file, and remove the #include "wdf/wdf.h" line. You might also need to tweak the file paths a little bit depending on which include paths are set by your build system.

@Murf
Copy link
Author

Murf commented Mar 4, 2023

I am going down that rabbit hole now and not having much joy. I will keep you posted.
Can you please explain how the strings are used in the template and maybe I can work around it?

@jatinchowdhury18
Copy link
Contributor

I'm not sure I understand the question. As I mentioned earlier, the templated API does not depend on the <string> header, or use strings in any way. This should also be evident from reading the code and documentation.

Would it be possible to share a little bit more about the build system that you're working, or maybe a small example project? I haven't worked with Reason before, so there may be additional relevant information that I'm not aware of.

Assuming that your build system has the include/chowdsp_wdf directory in its header search paths, the includes inside your copy of the main header file should only have to change from #include "wdft/wdft.h" to #include "chowdsp_wdf/wdft/wdft.h"

@Murf
Copy link
Author

Murf commented Mar 4, 2023

Good point, I should step back and explain a bit better.
Firstly, the environment is pretty basic, it is a SDK that Reason Studios provide and it is using Visual Studio at the command line to do the compiling (I have VS2019).

One thing it does not allow is dynamic memory allocation, but I have not really seen any in your library so far so I dont think that will be an issue.

When I include "chowdsp_wdf/chowdsp_wdf.h" with no changes I get the following error:
./wdf/chowdsp_wdf/wdf/wdf_base.h:4:10: fatal error: 'string' file not found
#include

I comment it out and it resolves with VS2019, then I get this error:
In file included from ./wdf/chowdsp_wdf/wdf/wdf_one_ports.h:4:
./wdf/chowdsp_wdf/wdf/wdf_base.h:18:35: error: implicit instantiation of undefined template 'std::__1::basic_string<char,
std::__1::char_traits, std::__1::allocator >'
explicit WDF (std::string type) : type (std::move (type)) {}

I assumed it was because string could still not be found BUT looking closer now I think this maybe something else?

Murf.

@jatinchowdhury18
Copy link
Contributor

Okay, that's good to know.

There' shouldn't be anything problematic about building the library with VS2019, since the library is tested regularly in that build environment.

I comment it out and it resolves with VS2019, then I get this error:
In file included from ./wdf/chowdsp_wdf/wdf/wdf_one_ports.h:4:
./wdf/chowdsp_wdf/wdf/wdf_base.h:18:35: error: implicit instantiation of undefined template 'std::__1::basic_string<char,
std::__1::char_traits, std::__1::allocator >'
explicit WDF (std::string type) : type (std::move (type)) {}

This error basically means that std::basic_string<char> (which is usually aliased as std::string for convenience) is not defined anywhere, which makes sense since normally that template would be defined in the <string> header.

You're correct that the chowdsp_wdf library does not do any memory allocation directly, but there are some C++ STL data structures (like std::vector and std::string that do memory allocation internally). In this library the string-related memory allocation is not really an issue since the string methods are not intended to be used in a real-time context (rather, they might be used when building a WDF at run-time, likely on a UI thread).

In any case, the method I had suggested above (making a modified copy of the main header), should be the simplest solution.

@Murf
Copy link
Author

Murf commented Mar 5, 2023

Assuming that your build system has the include/chowdsp_wdf directory in its header search paths, the includes inside your copy of the main header file should only have to change from #include "wdft/wdft.h" to #include "chowdsp_wdf/wdft/wdft.h"

I dont quite understand this sorry, there is only one copy of wdft.h in the whole library, this implies I am changing the main header file to look at a different version of this file?

@jatinchowdhury18
Copy link
Contributor

Right, so a couple things:

  • You should not change the main header file... you should copy it, place the copy somewhere within your own source tree and then change it. This will help avoid conflicts later on.
  • You're correct that there is only one copy of "wdft/wdft.h".
  • When the main header file include "wdft/wdft.h" the build system is finding that include relative to the file currently being parsed (in this case /repo/include/chowdsp_wdf/chowdsp_wdf.h).
  • After you make a copy of the main header file, and put your copied header file somewhere in your own source tree, that relative file path will no longer exist.
  • In order to make sure that the correct files are being included, you should edit the include directives in your copied header file so that they point to the same files, either using path relative to the location of your copied header, or relative to your build system's include paths.

This Stack Overflow thread has some more information on different types of file includes.

It's difficult to debug the specifics of your situation without knowing a bit more about how your source tree and other dependencies are set up.

@Murf
Copy link
Author

Murf commented Mar 5, 2023

Cool makes sense.
This is my structure so far (let's leave copying the header file for the time being, and address that later)


∨ ROOT
    ∨ wdf
        PassiveLPF.h (from your examples) - had to change header include from #include <chowdsp_wdf/chowdsp_wdf.h> to #include "chowdsp_wdf/chowdsp_wdf.h"
        ∨ chowdsp_wdf (this is a literal copy and paste of your Library repo chowdsp_wdf/include/chowdsp_wdf folder)
            > math
            > rtype
            > util
            > wdf
            > wdft
            chowdsp_wdf.h
            README.md
MyMainFile.h <- this has #include "wdf/PassiveLPF.h"
MyMainFile.cpp

I hope this structure looks ok in Text

@Murf
Copy link
Author

Murf commented Mar 5, 2023

Ok I think I got the change from wdf to wdft happening, with just these 2 header file changes (I no longer get a complaint about strings)

$ git diff
diff --git a/include/chowdsp_wdf/chowdsp_wdf.h b/include/chowdsp_wdf/chowdsp_wdf.h
index 1e69980..2674de4 100644
--- a/include/chowdsp_wdf/chowdsp_wdf.h
+++ b/include/chowdsp_wdf/chowdsp_wdf.h
@@ -20,7 +20,7 @@ constexpr int CHOWDSP_WDF_DEFAULT_SIMD_ALIGNMENT = 16;
 #endif

 #include "wdft/wdft.h"
-#include "wdf/wdf.h"
+// #include "wdf/wdf.h"
 #include "rtype/rtype.h"

 #include "util/defer_impedance.h"
diff --git a/include/chowdsp_wdf/rtype/rtype.h b/include/chowdsp_wdf/rtype/rtype.h
index bd8817b..2779f65 100644
--- a/include/chowdsp_wdf/rtype/rtype.h
+++ b/include/chowdsp_wdf/rtype/rtype.h
@@ -1,8 +1,8 @@
 #ifndef CHOWDSP_WDF_RTYPE_H_INCLUDED
 #define CHOWDSP_WDF_RTYPE_H_INCLUDED

-#include "rtype_adaptor.h"
+// #include "rtype_adaptor.h"
 #include "root_rtype_adaptor.h"
-#include "wdf_rtype.h"
+// #include "wdf_rtype.h"

 #endif // CHOWDSP_WDF_RTYPE_H_INCLUDED

I am still getting NaN all the time, and it is an unchanged copy of PassiveLPF.h that I am using.
Getting closer!

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

No branches or pull requests

2 participants