Enable Windows build of habitat-sim #84

cegbertOculus · 2019-06-20T23:37:42Z

Motivation and Context

The Windows build was not functioning. This change enables Windows builds.

How Has This Been Tested

Built on Windows with "python setup.py install" and run with "build/viewer /path/to/data/scene_datasets/habitat-test-scenes/skokloster-castle.glb"

Types of changes

Docs change / refactoring / dependency upgrade
[x ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have completed my CLA (see CONTRIBUTING)
I have added tests to cover my changes.
All new and existing tests passed.

src/esp/assets/PTexMeshData.cpp

src/esp/assets/FRLInstanceMeshData.cpp

src/esp/assets/GenericInstanceMeshData.cpp

src/esp/gfx/PTexMeshShader.cpp

mosra · 2019-06-21T00:19:19Z

src/CMakeLists.txt

@@ -7,7 +7,7 @@ cmake_minimum_required(VERSION 3.10)
 project(esp)

 if(MSVC)
-  add_definitions(/DNOMINMAX)
+  add_definitions(/DNOMINMAX  /D_USE_MATH_DEFINES /DEIGEN_DONT_ALIGN_STATICALLY)


What is _USE_MATH_DEFINES needed for? Does Habitat code need it or rather some dependency? If Habitat, we have our own constants for that, if 3rd party code, I'd prefere to have that specified in src/deps/CMakeLists.txt instead.

Three projects (nav, gfx, assets, all in src/esp) try to include deps/sophus/sophus/common.hpp and fail to build because M_PI is undefined.

I could define _USE_MATH_DEFINES in each of their respective CMakeLists.txt?

@cegbertOculus if sophus is the only culprit, perhaps we can have the define go along with the inclusion of sophus at https://github.com/facebookresearch/habitat-sim/blob/master/src/cmake/dependencies.cmake#L26

From what I can tell, the inclusion of sophus in dependencies.cmake just specifies where to include the sophus files from, and I can't define anything with that inclusion. Maybe it would make sense to put it in src/cmakelists.txt where the projects are added? Like this:

add_subdirectory(esp/nav)
if(MSVC)
target_compile_definitions(nav PUBLIC _USE_MATH_DEFINES )
endif()

Ah, you're right, I believe. Yes, it should be fine to add conditional target defines as above. Perhaps also with a short comment above the block stating that this is done for the sophus dependency, just for readability

Looking at Sophus' own CMakeLists, it sets it implicitly for the sophus target -- so I think the cleanest option is to explicitly link to the sophus target instead of relying just on it being inside the global include path? Like

target_link_libraries(nav PUBLIC ... sophus)

and similar for gfx and assets. Then we could even remove the global include path for Sophus, I think -- in any case I'm planning to do that as part of #42.

Explicitly linking didn't work, so I've just added:

if(MSVC) # Define common math constants if we compile with MSVC target_compile_definitions(assets INTERFACE _USE_MATH_DEFINES) endif (MSVC)

to the cmakelists.txt for nav, gfx, and assets. Is that acceptable?

PUBLIC instead of PRIVATE? I assume those definitions are needed when compiling the library itself as well. And please mention it's because of Sophus, otherwise this knowledge gets lost and somebody might remove it again in the future.

src/esp/gfx/WindowlessContext.cpp

cegbertOculus · 2019-06-28T02:01:56Z

Thanks for the feedback so far. I'll be out of the office for 2 weeks, so I'm putting this on hold for now, but I will pick it up again in 2 weeks.

cegbertOculus · 2019-08-28T16:47:53Z

New revision pushed. All issues have been resolved. However, this won't yet run on Windows as some changes are needed in magnum-bindings (mosra/magnum-bindings#2).

mosra

The mosra/magnum-bindings#2 part is merged now, so this could be ready to land -- can you update the corrade/magnum/... submodules to latest to pull those changes?

mosra · 2019-09-04T14:41:34Z

src/esp/assets/CMakeLists.txt

+if(MSVC)
+  # Define common math constants if we compile with MSVC.  This is needed because of Sophus.
+  target_compile_definitions(assets PUBLIC _USE_MATH_DEFINES)
+endif (MSVC)


Just endif(), repeating the condition is considered a bad practice nowadays.

Same in the other two cases.

lostmsu · 2019-10-30T23:21:39Z

@cegbertOculus merged your changes on top of v0.1.3, also replaced most usages of shlex (which I suspect is invalid for Windows paths) with passing simple lists of arguments. See https://github.com/lostmsu/habitat-sim/commits/windows

However, command-line build on Windows from a conda environment fails due to a problem with compile_commands.json:

...
-- Found Magnum: <CLONE_PATH>/src/deps/magnum/src
-- Configuring done
-- Generating done
-- Build files have been written to: <CLONE_PATH>/build/Release
error: [Errno 2] No such file or directory: 'build\\Release\\compile_commands.json'

compile_commands.json are nowhere to be found.

It generates the solution file though, which I can build from Visual Studio. But after that I am unsure how to proceed with setup.

erikwijmans · 2019-10-31T08:44:40Z

The logic here: https://github.com/facebookresearch/habitat-sim/blob/master/setup.py#L323
likely needs to be updated for windows.

HeterCol · 2020-01-09T05:26:14Z

@cegbertOculus merged your changes on top of v0.1.3, also replaced most usages of shlex (which I suspect is invalid for Windows paths) with passing simple lists of arguments. See https://github.com/lostmsu/habitat-sim/commits/windows

However, command-line build on Windows from a conda environment fails due to a problem with compile_commands.json:
...
-- Found Magnum: <CLONE_PATH>/src/deps/magnum/src
-- Configuring done
-- Generating done
-- Build files have been written to: <CLONE_PATH>/build/Release
error: [Errno 2] No such file or directory: 'build\\Release\\compile_commands.json'
compile_commands.json are nowhere to be found.

It generates the solution file though, which I can build from Visual Studio. But after that I am unsure how to proceed with setup.

Excuse me, I want to know how the problem 'compile_commands.json does not exist' can be solved. I have meet the same problem.

lostmsu · 2020-01-09T06:12:54Z

@liuxinzhu0353150307 I simply opened solution file under build\Release (I think it was esp.sln) in Visual Studio and finished the build there.

HeterCol · 2020-01-09T06:31:13Z

@liuxinzhu0353150307 I simply opened solution file under build\Release (I think it was esp.sln) in Visual Studio and finished the build there.

So have you built it successfully?And you can run the platform on Windows?

HeterCol · 2020-01-09T06:36:49Z

@liuxinzhu0353150307 I simply opened solution file under build\Release (I think it was esp.sln) in Visual Studio and finished the build there.

So have you built it successfully?And you can run the platform on Windows?

Actually, I don't find the .sln file in 'buile\Release', so I don't know how to do next.

cegbertOculus · 2020-01-09T17:45:47Z

@liuxinzhu0353150307 My memory is a little hazy, but I think I just made an empty compile_commands.json file.

HeterCol · 2020-01-10T02:03:15Z

@liuxinzhu0353150307 My memory is a little hazy, but I think I just made an empty compile_commands.json file.

so I only need to create an empty file 'compile_commands.json'?

cegbertOculus · 2020-01-10T19:24:33Z

so I only need to create an empty file 'compile_commands.json'?

I think it just needs one line with:
[]

eundersander · 2021-09-18T15:08:53Z

Closing this old PR. We can create a new PR from this branch if there's renewed interest.

anas-zafar · 2023-12-06T15:00:31Z

@eundersander , are we able to run this on windows?

facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Jun 20, 2019

mosra reviewed Jun 21, 2019

View reviewed changes

cegbertOculus force-pushed the master branch 2 times, most recently from 020eb07 to 2a9deaa Compare June 21, 2019 16:35

dhruvbatra mentioned this pull request Jul 8, 2019

path separator issue on microsoft winsuck #98

Closed

This was referenced Jul 15, 2019

Update Magnum and Python bindings #104

Merged

Switch to a multiplatform mmap implementation. #106

Merged

erikwijmans mentioned this pull request Aug 3, 2019

How to install Habitat on Windows? #127

Closed

mosra mentioned this pull request Aug 7, 2019

Support Rendering Replica models in the simulator (under CONSTRUCTION) #132

Closed

9 tasks

mosra mentioned this pull request Aug 27, 2019

Lots of warnings and all warnings as errors for esp code #174

Merged

11 tasks

cegbertOculus force-pushed the master branch 7 times, most recently from 849c90a to b1f09bd Compare August 28, 2019 00:17

cegbertOculus force-pushed the master branch from b1f09bd to c7fd6a7 Compare August 28, 2019 23:06

Enable Windows build of habitat-sim

1658ae3

cegbertOculus force-pushed the master branch from c7fd6a7 to 1658ae3 Compare August 28, 2019 23:08

mosra reviewed Sep 4, 2019

View reviewed changes

mosra mentioned this pull request Sep 17, 2019

Update Magnum for the latest goodies #218

Merged

erikwijmans mentioned this pull request Dec 16, 2019

A CMake Error while running build_ext #381

Closed

erikwijmans mentioned this pull request Jan 8, 2020

The cmake error when installing Habitat #416

Closed

erikwijmans mentioned this pull request Feb 24, 2020

Windows 10 support? facebookresearch/habitat-lab#313

Closed

eundersander closed this Sep 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Windows build of habitat-sim #84

Enable Windows build of habitat-sim #84

cegbertOculus commented Jun 20, 2019

mosra Jun 21, 2019

cegbertOculus Jun 21, 2019 •

edited

Loading

msavva Jun 21, 2019

cegbertOculus Jun 21, 2019 •

edited

Loading

msavva Jun 21, 2019

mosra Jun 22, 2019

cegbertOculus Aug 23, 2019

mosra Aug 27, 2019

cegbertOculus commented Jun 28, 2019

cegbertOculus commented Aug 28, 2019

mosra left a comment

mosra Sep 4, 2019

lostmsu commented Oct 30, 2019 •

edited

Loading

erikwijmans commented Oct 31, 2019

HeterCol commented Jan 9, 2020

lostmsu commented Jan 9, 2020 •

edited

Loading

HeterCol commented Jan 9, 2020

HeterCol commented Jan 9, 2020

cegbertOculus commented Jan 9, 2020

HeterCol commented Jan 10, 2020

cegbertOculus commented Jan 10, 2020

eundersander commented Sep 18, 2021

anas-zafar commented Dec 6, 2023

Enable Windows build of habitat-sim #84

Enable Windows build of habitat-sim #84

Conversation

cegbertOculus commented Jun 20, 2019

Motivation and Context

How Has This Been Tested

Types of changes

Checklist

mosra Jun 21, 2019

Choose a reason for hiding this comment

cegbertOculus Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

msavva Jun 21, 2019

Choose a reason for hiding this comment

cegbertOculus Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

msavva Jun 21, 2019

Choose a reason for hiding this comment

mosra Jun 22, 2019

Choose a reason for hiding this comment

cegbertOculus Aug 23, 2019

Choose a reason for hiding this comment

mosra Aug 27, 2019

Choose a reason for hiding this comment

cegbertOculus commented Jun 28, 2019

cegbertOculus commented Aug 28, 2019

mosra left a comment

Choose a reason for hiding this comment

mosra Sep 4, 2019

Choose a reason for hiding this comment

lostmsu commented Oct 30, 2019 • edited Loading

erikwijmans commented Oct 31, 2019

HeterCol commented Jan 9, 2020

lostmsu commented Jan 9, 2020 • edited Loading

HeterCol commented Jan 9, 2020

HeterCol commented Jan 9, 2020

cegbertOculus commented Jan 9, 2020

HeterCol commented Jan 10, 2020

cegbertOculus commented Jan 10, 2020

eundersander commented Sep 18, 2021

anas-zafar commented Dec 6, 2023

cegbertOculus Jun 21, 2019 •

edited

Loading

cegbertOculus Jun 21, 2019 •

edited

Loading

lostmsu commented Oct 30, 2019 •

edited

Loading

lostmsu commented Jan 9, 2020 •

edited

Loading