Skip to content

feat: add CMake support + build on Clang/Windows + build on C++20 + build on Windows ARM64 - #28

Closed
aminya wants to merge 23 commits into
mcneel:7.xfrom
aminya:cmake
Closed

feat: add CMake support + build on Clang/Windows + build on C++20 + build on Windows ARM64#28
aminya wants to merge 23 commits into
mcneel:7.xfrom
aminya:cmake

Conversation

@aminya

@aminya aminya commented Nov 16, 2021

Copy link
Copy Markdown
Contributor

I have split this into 7 independent pull requests as you requested:
#29 #30 #31 #32 #33 #34 #35


  • This pull request adds CMake support for OpenNURBS. This significantly simplifies the building and integration of OpenNURBS. The CMake files consider multiple platforms and all the dependencies.
  • It fixes a bug in opennurbs_gl where ON_Material::MaxShine which is a constant was being called as a function.
C:/opennurbs/opennurbs_gl.cpp:603:71: error: called object type 'double' is not a function or function pointer
    GLint shine = (GLint)(128.0*(pMat->Shine() / ON_Material::MaxShine()));
                                                 ~~~~~~~~~~~~~~~~~~~~~^
  • It fixes the bugs related to not-initializing constants where default constructors are defaulted
/home/aminya/opennurbs/opennurbs_statics.cpp:747:21: error: uninitialized const ‘ON_AngleValue::Unset’ [-fpermissive]
  747 | const ON_AngleValue ON_AngleValue::Unset ON_CLANG_CONSTRUCTOR_BUG_INIT(ON_AngleValue);
      |                     ^~~~~~~~~~~~~
In file included from /home/aminya/opennurbs/opennurbs.h:81,
                 from /home/aminya/opennurbs/opennurbs_statics.cpp:1:
/home/aminya/opennurbs/opennurbs_string_value.h:274:16: note: ‘const class ON_AngleValue’ has no user-provided default constructor
  274 | class ON_CLASS ON_AngleValue
      |                ^~~~~~~~~~~~~
/home/aminya/opennurbs/opennurbs_string_value.h:277:3: note: constructor is not user-provided because it is explicitly defaulted in the class body
  277 |   ON_AngleValue() = default;
      |   ^~~~~~~~~~~~~
C:/opennurbs/opennurbs_mesh.cpp:14834:34: error: no member named 'unique' in 'std::shared_ptr<ON_Mesh>'
    if (false == item->m_mesh_sp.unique())
                 ~~~~~~~~~~~~~~~ ^
  • It fixes a bug where 0xFFFFFFFF was being narrowed down to int, which resulted in an error in the Clang compiler. This was fixed by adding unsigned int to the enums:
C:/opennurbs/opennurbs_defines.cpp:2032:8: error: case value evaluates to -1, which cannot be narrowed to type 'unsigned int' [-Wc++11-narrowing]
  case os_all_snaps:     osm = os_all_snaps; break;
  • Fixes build on Windows ARM64. This involved fixing the preprocessor (e.g. adding _M_ARM64) and fixing some string conversions such as:
C:\opennurbs\opennurbs_version_number.cpp(378,12): error C2445: result type of conditional expression is ambiguous: types 'const ON_String' and 'const char [2]' can be converted to multiple common types

✅ Successfully tested on:

  • Windows using:
    • MSVC
    • Clang
    • shared library with MSVC
  • Linux using:
    • GCC
    • Clang
  • MacOS using:
    • Apple-Clang
    • Clang

There is also a pull request to add vcpkg support for opennurbs based on the current pull request
microsoft/vcpkg#21521

Here is the Azure pipeline of vcpkg testing the builds:

https://dev.azure.com/vcpkg/public/_build/results?buildId=63424&view=results

image

@aminya
aminya force-pushed the cmake branch 2 times, most recently from 5677ed7 to 3d8e3b8 Compare November 16, 2021 04:30
@aminya
aminya force-pushed the cmake branch 8 times, most recently from 2c4ca30 to 7667ea4 Compare November 16, 2021 05:56
@aminya
aminya force-pushed the cmake branch 3 times, most recently from c0274d0 to e56b6d8 Compare November 18, 2021 16:17
This adds the dirent library for windows + clang.
https://github.com/tronkko/dirent
Update CMakeLists.txt
Fixes
```
/home/aminya/opennurbs/opennurbs_statics.cpp:747:21: error: uninitialized const ‘ON_AngleValue::Unset’ [-fpermissive]
  747 | const ON_AngleValue ON_AngleValue::Unset ON_CLANG_CONSTRUCTOR_BUG_INIT(ON_AngleValue);
      |                     ^~~~~~~~~~~~~
In file included from /home/aminya/opennurbs/opennurbs.h:81,
                 from /home/aminya/opennurbs/opennurbs_statics.cpp:1:
/home/aminya/opennurbs/opennurbs_string_value.h:274:16: note: ‘const class ON_AngleValue’ has no user-provided default constructor
  274 | class ON_CLASS ON_AngleValue
      |                ^~~~~~~~~~~~~
/home/aminya/opennurbs/opennurbs_string_value.h:277:3: note: constructor is not user-provided because it is explicitly defaulted in the class body
  277 |   ON_AngleValue() = default;
      |   ^~~~~~~~~~~~~
```
@aminya aminya changed the title feat: add CMake support for OpenNURBS + build on Clang + build on C++20 feat: add CMake support + build on Clang/Windows + build on C++20 + build on Windows ARM64 Nov 20, 2021
@aminya
aminya requested a review from dalefugier November 20, 2021 02:26
starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Nov 22, 2021
@aminya

aminya commented Nov 22, 2021

Copy link
Copy Markdown
Contributor Author

@dalefugier The Microsoft/vcpkg team wants to see if @mcneel plans to merge this pull request. Otherwise, they will use my fork.

@dalefugier

Copy link
Copy Markdown
Member

@aminya - this specific PR is not going to be merged directly into openNURBS. It contains too many changes, and we're not comfortable accepting all of of them.

We will evaluate some of the changes you've made and see if they can be integrated into our private version of openNURBS, which is used by Rhino. If so, they will show up here when we publish updates to this, the open source version.

Also, adding CMake support is on our to-list. This is something we want to do.

As for additional compiler support, we'd need to run a number of tests to verify the integrity of file I/O before publishing.

Feel free to contact me, dale@mcneel.com, if you have any questions.

Thanks,

-- Dale

@sbaer

sbaer commented Nov 23, 2021

Copy link
Copy Markdown
Member

Microsoft is going to use an unofficial fork for their vcpkg distribution? This is a big PR and there are several modifications that I am not comfortable with which I would like to spend time investigating. The first set of issues I see are

  • the use of glob for cmakelists.txt. This is typically not a recommended approach
  • declaring some enums to be unsigned int
  • using static_cast for some ON_wStrings
  • modifications to a bunch of ifdefs

I appreciate the work, but we need to step back and try to tackle this in much smaller and manageable bits.

@aminya

aminya commented Nov 23, 2021

Copy link
Copy Markdown
Contributor Author

Thanks for the responses.

I have included the rationale behind each of the changes in the pull request description. To remove any confusion, the exact compiler error for each of the changes is included. The changes are tested in the Microsoft Azure CI system, which I have included the link for it in the above description.

Unlike some of the commits I saw in this repository, I used bit-sized commits and included the description for each change following conventional commit messages. So if you want me to break them down into separate pull requests, I will do so.

@sbaer

sbaer commented Nov 23, 2021

Copy link
Copy Markdown
Member

We do occasional large commits from our private repo version of opennurbs to keep things in sync. This is why you see some large commits.

@OmidAstmal OmidAstmal left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thank you so much for this pull request. Your description of the changes is very detailed and makes sense.
I am surprised that the maintainers of this repository say that these are not explained enough.
Our team has been using OpenNURBS in production for a long time, and we were always wondering why @mcneel does not use this repository, and it is just a place for dumping whatever they use in private. Open-source programming is not just about adding an "Open" prefix to the name. It also means being open to the contribution of external people.
If @mcneel does not plan to merge these changes, our team will switch to this alternative fork.

@dalefugier

dalefugier commented Dec 1, 2021

Copy link
Copy Markdown
Member

The "open" is openNURBS indicates that Rhino's 3dm file format is open and not proprietary. So open that we provide the same source code, used by Rhino, for reading and writing this file format.

We first made the source code to openNURBS available in 2000, and it used to be distributed by way of a zip file. We had several requests for distribution via GitHub repository. So here it is.

We do accept pull requests. Each one is carefully deliberated and extensively tested, as the integrity of 3dm file reading and writing is paramount.

This pull request was rejected for the reason's we've already mentioned. However, you're welcome to fork this repository - you won't be the first.

-- Dale

@sbaer

sbaer commented Dec 1, 2021

Copy link
Copy Markdown
Member

@aminya please break this down into separate pull requests. There is too much change here to be able to review and merge with any level of confidence.

@aminya

aminya commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

Sure, I will do so in a few days. I will split the changes into a few separate pull requests.

@aminya

aminya commented Dec 3, 2021

Copy link
Copy Markdown
Contributor Author

@sbaer I have split this into 7 independent pull requests as you requested.
#29 #30 #31 #32 #33 #34 #35

@sbaer sbaer closed this Dec 28, 2021
starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Jan 21, 2022
starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Feb 15, 2022
starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Jul 14, 2022
starseeker added a commit to BRL-CAD/brlcad that referenced this pull request Sep 8, 2022
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.

4 participants