Skip to content
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

Updates for SketchUp 2024 #714

Merged
merged 33 commits into from
May 30, 2024
Merged

Updates for SketchUp 2024 #714

merged 33 commits into from
May 30, 2024

Conversation

macumber
Copy link
Collaborator

@macumber macumber commented May 17, 2024

@jmarrec if you want to look at getting the SketchUp Plug-In working on Mac maybe we could both work on this branch

The current issue I am seeing is in QCoreApplication* ApplicationSingleton::application(bool gui), getOpenStudioModuleDirectory is returning a bad path object.

@macumber
Copy link
Collaborator Author

macumber commented May 18, 2024

It's loading in a debug build which doesn't help debug the issue but does show we're not too far off

image

@macumber
Copy link
Collaborator Author

macumber commented May 18, 2024

Downgrading our version of Qt to 6.5.3 to match SketchUp (6.5.2) has enabled the plug-in to load for me on Windows. Really would be nice to get Qt out of the SketchUp Plug-in and just use native SketchUp dialogs. Have to rebuild the inspector gadget...

See openstudiocoalition/openstudio-sketchup-plugin#133 and https://github.com/openstudiocoalition/openstudio-sketchup-plugin/tree/html_inspector


jobs:
build:
# Note: as of 2021-01-29, this only works for push, not for pull request
# if: "!(contains(github.event.head_commit.message, 'skip') && contains(github.event.head_commit.message, 'ci'))"
if: ${{ github.event_name == 'push' || !github.event.pull_request.draft }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jmarrec what do you think about this? I can't remember when we would want to use 'skip' or 'ci' instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm the way I have it now marking as "Ready to Review" doesn't trigger the build, I'll work on that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok now it runs when marked ready for review

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you can push a commit with "[skip ci]" in the commit message and the run won't be triggered.

But this is now baked into github actions anyways; https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs

@macumber macumber marked this pull request as ready for review May 18, 2024 17:10
@macumber macumber marked this pull request as draft May 18, 2024 17:11
@macumber macumber marked this pull request as ready for review May 19, 2024 16:05
@macumber
Copy link
Collaborator Author

@jmarrec I also updated to official 3.8.0 here


env:
BUILD_TYPE: Release
BUILD_DOCUMENTATION: OFF
BUILD_TESTING: ON
BUILD_BENCHMARK: ON
BUILD_PACKAGE: ON
QT_VERSION: 6.6.3
QT_VERSION: 6.5.3
Copy link
Collaborator

@jmarrec jmarrec May 19, 2024

Choose a reason for hiding this comment

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

Yikes. That's for sketchup using a different Qt then. How was it working before though? Is Qt a new sketchup dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah I don't really like having our Qt version tied to SketchUp's. That is the idea of eventually removing all the Qt stuff from the SketchUp Plug-in, the biggest thing to replace is the Inspector Dialog which is not that hard but it isn't trivial.

@@ -8,19 +8,20 @@ on:
- 'v*' # Push events to matching v*, i.e. v1.0, v20.15.10
pull_request:
branches: [ master, develop ]
types: [ opened, reopened, synchronize, ready_for_review ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you want to add this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They default types for pull_requests are [ opened, reopened, synchronize ]. I added ready_for_review in the case that a PR is draft and then you mark it ready for review without pushing a commit. The idea is if you want to push code to a branch but not kick off a bunch of CI builds you can mark the PR as draft. Then when you want CI to run you set as ready for review. To me that seems nicer than adding [skip ci] to each commit message but we could do that too.


jobs:
build:
# Note: as of 2021-01-29, this only works for push, not for pull request
# if: "!(contains(github.event.head_commit.message, 'skip') && contains(github.event.head_commit.message, 'ci'))"
if: ${{ github.event_name == 'push' || !github.event.pull_request.draft }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

So you can push a commit with "[skip ci]" in the commit message and the run won't be triggered.

But this is now baked into github actions anyways; https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs

.github/workflows/app_build.yml Outdated Show resolved Hide resolved
@macumber
Copy link
Collaborator Author

I like your change @jmarrec, you good to merge this or do you want to think some more on it?

@jmarrec
Copy link
Collaborator

jmarrec commented May 22, 2024

I have a vague recollection of us bumping Qt 6 version to avoid a bug or two (display all black on macos with dark theme maybe?). it does pain me to downgrade qt (I'd rather bump up 6.7.x), but i really just want to make sure we aren't re-creating problems.

Do you have a guesstimate of the effort needed to port the IG / model_editor to native sketchup so we avoid binary mismatch ?

@macumber
Copy link
Collaborator Author

My guess is 15-20 hours for the inspector replacement, it would be a significant effort so I'd want to clear it with the OSC first. If it's worthwhile we can ask if that is in scope for next release. Otherwise we can go with this approach and maybe do the html inspector later. There will always be a binary compatibility issue with OpenStudio SDK and SketchUp due to Ruby version.

@jmarrec
Copy link
Collaborator

jmarrec commented May 23, 2024

@macumber There's a decent change this will break this again:

Is packaging the .so within the sketchup-plugin instead of asking for a path to the OSApp possible?

If so, perhaps we could be making the Qt version configurable, and only use 6.5.3 in specific workflows and use those binaries for Sketchup-plugin.
Obviously that would create a hassle for maintaining (gotta compile with both versions to ensure you don't use a method that's in Qt 6.7 and not in 6.5).

Comment on lines 54 to 59
if !File.exist?(File.join(qt_dll_path, 'Qt5Core.dll'))
release_dll_path = File.expand_path(File.join(File.dirname(__FILE__), '../../Release/'))
debug_dll_path = File.expand_path(File.join(File.dirname(__FILE__), '../../Debug/'))
if File.exists?(File.join(release_dll_path, 'Qt5Core.dll'))
if File.exist?(File.join(release_dll_path, 'Qt5Core.dll'))
qt_dll_path = release_dll_path
elsif File.exists?(File.join(debug_dll_path, 'Qt5Core.dll'))
elsif File.exist?(File.join(debug_dll_path, 'Qt5Core.dll'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we looking for Qt5? I suppose "if install path fails" is always false then.

CMakeLists.txt Outdated
Comment on lines 515 to 523
set(QT_VERSION "6.5.3" CACHE STRING "Qt target version, defaults to 6.5.3")

# For AboutBox, but also validates that the version is valid
string(REGEX MATCH "^([0-9]+\\.[0-9]+)"
QT_VERSION_MAJOR_MINOR ${QT_VERSION})
if(NOT QT_VERSION_MAJOR_MINOR)
message(FATAL_ERROR "Failed to match QT_VERSION=${QT_VERSION}")
endif()
set(QT_DOC_LINK "https://doc.qt.io/qt-${QT_VERSION_MAJOR_MINOR}/qtmodules.html")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Allow passing QT_VERSION... default to 6.5.3

Make a link for the aboutbox

ruby/openstudio_modeleditor.rb Outdated Show resolved Hide resolved
src/openstudio_app/AboutBox.hpp.in Show resolved Hide resolved
@jmarrec
Copy link
Collaborator

jmarrec commented May 23, 2024

@bwinsatt @mioruggieroguida I'm worried that this PR will affect M2 users (#688). We do not have a M2/M3 machine for testing. Could you help us by testing the macos arm64 installer from https://github.com/openstudiocoalition/OpenStudioApplication/actions/runs/9204641443?pr=714 please? They are already up

@macumber
Copy link
Collaborator Author

SketchUp Plug-in is still not loading on Mac:

image

SketchUp is using Qt 6.5.2 and we are using 6.5.3, we may need to match their version exactly

@jmarrec
Copy link
Collaborator

jmarrec commented May 27, 2024

I had this error. sorry for the screenshot, I can't do anything else with this window open.

@bwinsatt unfortunately that's not our fault, it's the OS SDK, the "classic" subcommand for measure manager is broken

$ openstudio --version
3.8.0+f953b6fcaf
$ openstudio classic measure -s 8091
Error executing argv: ["measure", "-s", "8091"]
Error: cannot load such file -- webrick in eval:182:in `require'

webrick gem was removed from the standard library in Ruby 3...

Make sure you go to "Preferences" and uncheck the "Use Classic CLI" checkbox there. (@macumber we should probably remove it...)

@macumber
Copy link
Collaborator Author

Loading on SketchUp 2024 on Mac M1 now after matching Qt version exactly:

image

@macumber
Copy link
Collaborator Author

@bwinsatt can you try resetting your OpenStudio Application preferences manually using the workaround in #717 ?

Then after that, can you try downloading at using the package from:
https://github.com/openstudiocoalition/OpenStudioApplication/actions/runs/9258087440

Thank you so much for your help on this!

@macumber
Copy link
Collaborator Author

@jmarrec if @bwinsatt says this is good to go I think this is ready to merge.

@jmarrec
Copy link
Collaborator

jmarrec commented May 27, 2024

Yeah @macumber merge at will after that.

@bwinsatt
Copy link

Can you clarify what I need to do to test on M2? I was able to install and open the application after following #717.

After opening, I get this error, but the application still is usable after clicking ok.
image

@jmarrec
Copy link
Collaborator

jmarrec commented May 30, 2024

@bwinsatt That's all we needed, thanks!

that error will be resolved soon, it's an OS SDK issue.

@jmarrec jmarrec merged commit cf61365 into develop May 30, 2024
8 checks passed
@jmarrec jmarrec deleted the sketchup_2024 branch May 30, 2024 15:23
@github-actions github-actions bot locked and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants