Conversation
…s cubit version to iGeom.cpp
bam241
left a comment
bam241
left a comment
There was a problem hiding this comment.
few comments, otherwise this looks good!
thank @ljacobson64 !
pshriwise
left a comment
pshriwise
left a comment
There was a problem hiding this comment.
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
| 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.
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.
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 |
pshriwise
left a comment
pshriwise
left a comment
There was a problem hiding this comment.
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. |
3 participants
This PR combines some changes from @pshriwise and @bam241 to do a few things:
CUBIT_VERSIONwhich cmake uses to determine whether theCUBIT_17_PLUSdefinition 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.