Skip to content

add aortic_valve.cpp - #290

Open
yuanjiajy wants to merge 8 commits into
Xiangyu-Hu:masterfrom
yuanjiajy:aortic-valve
Open

add aortic_valve.cpp#290
yuanjiajy wants to merge 8 commits into
Xiangyu-Hu:masterfrom
yuanjiajy:aortic-valve

Conversation

@yuanjiajy

Copy link
Copy Markdown
Collaborator

No description provided.

@yuanjiajy yuanjiajy closed this May 18, 2023
@Xiangyu-Hu Xiangyu-Hu reopened this May 20, 2023
@Xiangyu-Hu

Xiangyu-Hu commented May 20, 2023

Copy link
Copy Markdown
Owner

@yuanjiajy you do not need to close the pull request, just transfer it draft request.
So that you can still work on it until ready for a real pull request.

@Xiangyu-Hu
Xiangyu-Hu marked this pull request as draft May 20, 2023 07:06
@Xiangyu-Hu Xiangyu-Hu added the enhancement New feature or request label May 20, 2023
@yuanjiajy
yuanjiajy marked this pull request as ready for review May 22, 2023 12:23
@Xiangyu-Hu

Copy link
Copy Markdown
Owner

CI need pass before merge.

@Xiangyu-Hu

Copy link
Copy Markdown
Owner

You are supposed not change the library directly.

@Xiangyu-Hu

Copy link
Copy Markdown
Owner

I guess the problem is due to the library libx11-dev and freeglut3-dev, may be some others, should be install first.

Comment thread .github/workflows/ci.yml
Comment on lines +41 to +42
libx11-dev \
freeglut3-dev \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Those dependencies are really orthogonal to this library. (opencascade requires actually libgl-dev rather than freeglut3-dev). If there is no lighter CAD kernel you can use for the purpose, I would recommend to contribute to vcpkg to modularize the opencascade installation, or use the FetchContent mechanism in this repo to get the TKSTEP target without spurious pieces.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Actually it doesn't work when i only install the libgl-dev, so i install libgl-dev and freeglut3-dev.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, it doesn't work because the whole of opencascade is installed without distinction, including optional features relying on freeglut. As mentioned in my previous comment, you would need to modularize the vcpkg port to avoid installing everything, or rely on CMake FetchContent mechanism to only build and link TKSTEP.

Comment thread CMakeLists.txt
Comment on lines +67 to +68
find_package(OpenCASCADE CONFIG REQUIRED)
target_link_libraries(sphinxsys_core INTERFACE TKSTEP )

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The module relying on occt should be independent so that the user can choose to enable or disable it with an option

Comment on lines +6 to +20
Vec3d OcctToEigen(const gp_Pnt &occt_vector)
{
return Vec3d(occt_vector.X(), occt_vector.Y(), occt_vector.Z());
}
//=================================================================================================//
gp_Pnt EigenToOcct(const Vec3d &eigen_vector)
{
return gp_Pnt(eigen_vector[0], eigen_vector[1], eigen_vector[2]);
}
//=================================================================================================//
Vec3d OcctVecToEigen(const gp_Vec &occt_vector)
{
return Vec3d(occt_vector.X(), occt_vector.Y(), occt_vector.Z());
}
//=================================================================================================//

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please move things out of src/shared to keep occt bounded out of the core lib at the moment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Please move things out of src/shared to keep occt bounded out of the core lib at the moment

Yes i have removed things to the extra source part in the new pull request

@yuanjiajy

Copy link
Copy Markdown
Collaborator Author

I guess the problem is due to the library libx11-dev and freeglut3-dev, may be some others, should be install first.

Yes and I have changed it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorporating the shell particle generation based on surface geometry

3 participants