-
Notifications
You must be signed in to change notification settings - Fork 14
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
Handle Trelis 17 #64
Handle Trelis 17 #64
Conversation
…s cubit version to iGeom.cpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments, otherwise this looks good!
thank @ljacobson64 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple requests for clarity that the change in the API is based on the SDK being used as opposed to the version of Cubit/Trelis.
CMakeLists.txt
Outdated
@@ -77,6 +85,9 @@ if (BUILD_IGEOM) | |||
include_directories(iGeom) | |||
add_library(iGeom SHARED ${IGEOM_SRC_FILES}) | |||
target_link_libraries(iGeom ${CUBIT_LIBS}) | |||
if (CUBIT_VERSION VERSION_GREATER_EQUAL 17.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a comment here noting that this really has to do with the SDK version as opposed to the version of Trelis/Cubit? As per @gonuke's comment #61 (comment) here, it still isn't clear what versions of Cubit (as opposed to Trelis versions) this will apply to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if
statement was removed per my comment on iGeom.cpp
…tead of requiring users to specify it
I just pushed 2 commits to my branch, and I can see them if I look at my branch, but they're not showing up on this PR. Am I losing my mind? |
Guess I'm not losing my mind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to me. Feeling much better about how the API change is being handled here. Thanks @ljacobson64!
Will merge at the end of the day unless there are further comments. |
This PR combines some changes from @pshriwise and @bam241 to do a few things:
CUBIT_VERSION
which cmake uses to determine whether theCUBIT_17_PLUS
definition should be used. I figure this is more forward compatible in case future versions of Trelis require slight changes as well.After this PR, everything should be set up properly for @bam241 to add his uwuw functionality.