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

Fix issues on MSVC and update build instructions. #6

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

speps
Copy link

@speps speps commented Feb 12, 2020

I wanted to run the demo in MSVC. Most of the instructions worked but I had some issues with missing ones and some CMake issues where the values were GCC/Clang specific.

@speps
Copy link
Author

speps commented Feb 12, 2020

The change in the data/* files is MSVC compiler not handling that data as std::vector well.

add_library(WaveGrid WaveGrid.cpp Enviroment.cpp Grid.cpp ProfileBuffer.cpp Spectrum.cpp ${Environment_RCS})
target_link_libraries(WaveGrid Eigen3::Eigen)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eigen3 does not have any libraries, it's header only?!

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's required to setup the correct include paths for the project. Maybe that's a MSVC thing but I don't think it hurts anyway.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -53,7 +53,7 @@ class WaterSurfaceMesh : public SceneBase3D::Object3D, public SceneGraph::Drawab
// std::vector<VertexData> newData = _data;

#pragma omp parallel for
for (size_t i = 0; i < _data.size(); i++) {
for (int i = 0; i < _data.size(); i++) {
Copy link

@VictorLamoine VictorLamoine Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? An int is not safe when iterating on a STL container, size_t is designed for this.
Ideally you would even use std::vector<Foo>::size_type (see https://stackoverflow.com/questions/45168636/stdsize-t-or-stdvectorfoosize-type) but most of the time std::size_t is fine (certainly better than a int).

Copy link
Author

@speps speps Feb 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found that MSVC even 2019 doesn't support OpenMP 3.0 so all for loops need to use a signed type. I disabled OpenMP on MSVC below in 49b116f, performance seemed very similar. And I reverted this line.

@WhizZest
Copy link

Hello @speps , I encountered the same issue when building this project with MSVC 2019, so I directly cloned your modified version. The project built and compiled successfully, but during runtime, the program crashed inside ImGui with the error message "g was nullptr," as shown in the screenshot below:
image
Call Stack:
image
The crash point of the project source code:
image

I haven't modified the source code, and I'm wondering if you've encountered this issue as well? I checked the usage of ImGui in the source code and didn't find any issues.

@WhizZest
Copy link

Well, this should be a bug of magnum-integration[imgui], I have solved it, using sdl2's imgui to replace magnum's imgui, a stopgap solution, currently saved in my git repository.
GIF 2023-12-29 20-08-39

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.

3 participants