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

Namespaces #551

Merged
merged 43 commits into from
Jul 8, 2021
Merged

Namespaces #551

merged 43 commits into from
Jul 8, 2021

Conversation

ptheywood
Copy link
Member

@ptheywood ptheywood commented Jun 3, 2021

Introduce C++ namespaces to the library, to avoid polluting the global namespace.

This is not as namespacey as I would like, however due to time / complexity it will do for now.

  • Everything goes into flamegpu namespace
  • Adds several sub-namespaces
    • exception - mostly just to clean up the docs
    • io - nicely self-contained.
    • util - quite self contained
    • `visualiser - distinct from the main repo, although there is some mixing.
    • detail namespaces - parts of headers that users ideally shouldn't know exists
      • Ideallt more would be in detail, but due to mutual friendship + how protected etc interact with namespaces this is more of a problem than desired.
    • Did not split out the host/device components of the API, too interlinked.
      • Flatten the file hierarchy for gpu, pop, runtime, sim? - 70+ headers currently...
        • A number of these should be in detail, but moving them to detail so far has generally broken things.
  • Full rewrite of the swig .i file to achieve working namespaces.
    • Now more well-structured based on the order in which different swig features need using.
    • Care needs to be taken when wrapping new classes, to ensure that swig is aware of the type prior to it being used. Forward declarations in included header files may need to be manually defined in some cases.
  • Renames many things, part of Class / Namespaces to Rename / Move #550
    • Not complete but decent amount done.
  • Rename headers which contain CUDA symbols .cuh.
  • Move some files around to more appropraite locations
  • Expand util with additional typedefs used as keys in maps (std::pair<X,Y> mostly)
  • New tests
    • Check that RTC Message type checkign correctly matches when the rtc string includes namespace flamegpu {, using namespace flamegpu, or using flamegpu::MsgNone etc)
    • Add tests to check that regular agent functions build using the various approaches to namespaces (explicit, using directive (maybe, cpplint will hate it), using declaration, alias, alias + explcicit)
  • Adds utiltiy method to get the unqualified class name from a (potentially) qualified class name, for error human friendly errors.
  • Remove os-platform from lib/bin directory paths. It was unneccesary.
  • Config specific python lib directories (closes Separate config specific directory for python builds #531)
  • Disables some rogue printf's in DependencyGraph.
  • Misc CMake improvements
    • More to be done.
  • Block some namespaces from appearing in doxygen.
  • CMake option BUILD_SWIG_PYTHON_VIRTUALENV -> BUILD_SWIG_PYTHON_VENV
  • Add missing __threadsync() to AgentFunctionWrapper when SEATBELTS is on. Closes Missing threadsync in the agent function wrapper #568.
  • Remove unused exceptions. Closes Unused Exception Types #578.
  • Adds python test checking ID_t works correctly (in test_cuda_simulation.py)
  • RTC Compile Time improvment(s), part of Reducing RTC compilation time #402.
    • Only include the required messaging types for each agent function. C++ test suite time 500s -> 355s.

Todo pre-merge:

  • Merge vis repo PR once re-tested

Todo pre/post-merge:

  • Open issues:

    • Extra namespacing / detail namespacing
    • Developer docs CMake option / target which includes things which are not part of the public API.
    • Message class names, i.e. MsgNone does not match messaging/None.h / messaging/None/NoneHost.h. Couldn't move this to messaging namespace because of mutual friendship (probably fixable, with effort / time, might cause issues with swig too, unsure).
    • Update docs.
  • Update each standalone repo to use the namespaces

@ptheywood
Copy link
Member Author

ptheywood commented Jun 11, 2021

With namespaces, the generated swig_types[] deifnes have the full flattened namespace in the name. If they have been encountered before that type is wrapped, it isn't prefixed by the appropraite namespaces, leading to something that builds but the generated python is incorrect.

The fix so far is to re-order the swig %includes to make sure this doesn't happen.

I expect this will eventually lead to a circular dependency hell kind of situation...

Currently the following are being generated without the appropriate namespace(s). They are sorted alphabetically so the order of these is not useful.

// In flamegpuPYTHON_wrap.cxx ~ 3100 lines in but may move around.
#define SWIGTYPE_p_Agent swig_types[0]
#define SWIGTYPE_p_AgentDataBuffer swig_types[1]
#define SWIGTYPE_p_AgentDataBufferStateMap swig_types[2]
#define SWIGTYPE_p_AgentDataMap swig_types[3]
#define SWIGTYPE_p_AgentInterface swig_types[4]
#define SWIGTYPE_p_AgentOffsetMap swig_types[5]
#define SWIGTYPE_p_CAgent swig_types[6]
#define SWIGTYPE_p_EnvironmentDescription swig_types[7]
#define SWIGTYPE_p_FGPURuntimeException swig_types[8]
#define SWIGTYPE_p_FLAMEGPU_HOST_FUNCTION_POINTER swig_types[9]
#define SWIGTYPE_p_HostAPI swig_types[10]
#define SWIGTYPE_p_ModelDescription swig_types[11]
#define SWIGTYPE_p_MsgBruteForce__Description swig_types[12]
#define SWIGTYPE_p_NameStatePair swig_types[13]
#define SWIGTYPE_p_RandomManager swig_types[14]

@ptheywood
Copy link
Member Author

Right now, a single pytest is failing in tests/swig/python/runtime/messaging/test_brute_force.py. Unclear why it is only one, need to investigate.

Building with vis enabled is currently generating some wonky types. Looks like either the vis swigging needs to occur earlier, or if vis is enabled some additional forward declarations might be required (possibly just the one of ModelVis, which is available via CUDASimulation?)

After that, python should work and actual work on additional namespacing / renaming things / sorting header file types / moving files around can resume.

@ptheywood
Copy link
Member Author

Oddly all tests are currently passing for VIS=OFF builds, unsure why the brute force test failiure is no longer occuring..

Updates all tests and examples to use these new namespaces

Complete rewrite of pyflamegpu.i to generate a single python module that uses the appropraite classes.

Adds some missing override statments

Adds new tests to check namespaces can be used in several ways

Improves some exception/error strings

Removes an unneccesary header dependency by moving a typedef.
This requires workarounds for CMake < 3.20, where genex cannot be used as byproducts
Instead, single-config generators use CMAKE_BUILD_TYPE.
multi-gen's do not specify byproducts, as the output path being consistent is more important
Unclear if byproducts actually have impact in vs/multi-generator configs in general

Fix byproducts with mulitgenerators and older cmake
CMake exclusively only allows a single platform to be specified for a given build directory, regardless of single or multi config generator
Potentially this could become flamegpu.cuh given it includes other headers which should be .cuh,
But in practice, we want users to just include .h when we eventually have a non-cuda build, and isntead the header should have #ifndef CUDACC to include the .cuh headers if being built with cuda support?
@ptheywood ptheywood marked this pull request as ready for review July 7, 2021 17:58
@ptheywood
Copy link
Member Author

ptheywood commented Jul 7, 2021

Once CI passes this should be in a mergable state. History is made up of meaningful isolated commits so i'd prefer a rebase / ff merge than a squash.
It needs to be done at the same time as the matching PR on the vis repo, and then I'll update all the standalone repos.

I've opened new issues to track the other outstanding changes which didn't make it in.

I've not changed the pop/sim/runtime/gpu directories in the end, unhappy with the alternatives but it shouldn't be a big deal to move these files around in the future (renameing / namespacing might be, but that is going to be a breaking change if / when it happens anyway)

@ptheywood ptheywood merged commit 49b4183 into master Jul 8, 2021
@ptheywood ptheywood deleted the namespaces branch July 8, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant