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

Try replacing Mux and WebSockets with pure HTTP.jl #241

Merged
merged 6 commits into from
May 21, 2023
Merged

Conversation

rdeits
Copy link
Owner

@rdeits rdeits commented May 15, 2023

Fixes #235 (by removing WebSockets.jl entirely).

Fixes #236, I hope (by allowing us to upgrade Blink.jl to v0.12.6). Edit: Blink.jl is gone, so the problem is gone too.

This PR rewrites the core web server to only use HTTP.jl, eliminating the dependencies on Mux.jl and WebSockets.jl. I'm hoping this will avoid the issues we've seen in recent versions of those packages (#234, #233), and it should allow us to benefit from the fact that HTTP.jl seems to be much more reliably updated.

We were previously using Mux.jl to handle routing the basic URLs that the meshcat javascript needs, but the HTTP.jl Router class works just as well. Dispatching streams to the HTTP or websocket handler is also pretty easy without using Mux.jl at all. And the actual WebSocket implementation is now provided by HTTP.jl, so we don't need WebSockets.jl at all.

The downside is that we have to upgrade the Julia requirement to 1.8 due to Blink requiring WebIO which requires WebSockets.jl (which we're not actually using) which requires Julia 1.8.2. Edit: Removed Blink.jl in favor of Electron.jl to fix this (fixes #242 ).

@rdeits
Copy link
Owner Author

rdeits commented May 15, 2023

@ferrolho would you mind helping me test this out? I've made sure the very basic stuff like opening (and re-opening) a visualizer works, but I won't have time to do much more in the next day or two.

@rdeits
Copy link
Owner Author

rdeits commented May 15, 2023

Turns out it was easy to eliminate the dependency on Blink.jl entirely, so I did that. We should be able to support Julia 1.6 again.

@ferrolho
Copy link
Collaborator

ferrolho commented May 15, 2023

Using the Julia REPL for the commands + Chrome to open the visualiser:

  • Basic opening/closing works;
  • README.md examples work;
  • Saving screenshot/scene works;
  • Loading a scene does not work;
    • I don't use this regularly, so it may have been broken before. To be checked.
  • Closing/reopening tab on same address works;
  • Starting a second visualiser from the same REPL works.

Using the Julia REPL for the commands + Electron to open the visualiser:

  • Just after running using Electron and app = Application();, I get the following output:

    objc[15894]: Class WebSwapCGLLayer is implemented in both /System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks/WebCore.framework/Versions/A/Frameworks/libANGLE-shared.dylib (0x23b4befe0) and /Users/henrique/.julia/artifacts/12f3018147190ddc494f686e5fbefe8d84f16efb/Julia.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libGLESv2.dylib (0x109a51348). One of the two will be used. Which one is undefined.

  • Then, running open(vis, app); gives me

    (node:16307) [DEP0005] DeprecationWarning: Buffer() is deprecated due to security and usability issues. Please use the Buffer.alloc(), Buffer.allocUnsafe(), or Buffer.from() methods instead.
    (Use Electron Helper (Renderer) --trace-deprecation ... to show where the warning was created)

  • Closing an Electron window and running a command will produce the following error:
    ERROR: IOError: write: broken pipe (EPIPE)
    Stacktrace:
      [1] uv_write(s::Sockets.TCPSocket, p::Ptr{UInt8}, n::UInt64)
        @ Base ./stream.jl:1066
      [2] unsafe_write(s::Sockets.TCPSocket, p::Ptr{UInt8}, n::UInt64)
        @ Base ./stream.jl:1120
      [3] unsafe_write
        @ ./io.jl:685 [inlined]
      [4] unsafe_write(s::Sockets.TCPSocket, p::Base.RefValue{UInt16}, n::Int64)
        @ Base ./io.jl:683
      [5] write
        @ ./io.jl:686 [inlined]
      [6] write
        @ ./io.jl:689 [inlined]
      [7] writeframe(io::HTTP.Connections.Connection{Sockets.TCPSocket}, x::HTTP.WebSockets.Frame)
        @ HTTP.WebSockets ~/.julia/packages/HTTP/A83Es/src/WebSockets.jl:184
      [8] send(ws::HTTP.WebSockets.WebSocket, x::Vector{UInt8})
        @ HTTP.WebSockets ~/.julia/packages/HTTP/A83Es/src/WebSockets.jl:519
      [9] write(core::MeshCat.CoreVisualizer, data::Vector{UInt8})
        @ MeshCat ~/git/MeshCat.jl/src/visualizer.jl:121
    [10] send
        @ ~/git/MeshCat.jl/src/visualizer.jl:129 [inlined]
    [11] settransform!(vis::Visualizer, tform::Translation{StaticArraysCore.SVector{3, Float64}})
        @ MeshCat ~/git/MeshCat.jl/src/visualizer.jl:199
    [12] top-level scope
        @ REPL[16]:1
    I'm not sure if we want to keep it like that, or fail silently somehow. It seems that re-opening another window using the same visualiser will fix the issue within the same Julia session.

I've also tested running stuff from a notebook on Jupyter lab, and it worked well too.

All the tests passed locally when running ] test from the project root. There are only a few deprecation warnings (for which we could open an issue):

┌ Warning: `rotation_between(from::AbstractVector, to::AbstractVector)` is deprecated, use `rotation_between(SVector{3}(from), SVector{3}(to))` instead.
│   caller = intrinsic_transform(g::Cylinder3{Float64}) at geometry.jl:76
└ @ MeshCat ~/git/MeshCat.jl/src/geometry.jl:76
┌ Warning: `rotation_between(from::AbstractVector, to::AbstractVector)` is deprecated, use `rotation_between(SVector{3}(from), SVector{3}(to))` instead.
│   caller = intrinsic_transform(g::Cone{3, Float64}) at geometry.jl:82
└ @ MeshCat ~/git/MeshCat.jl/src/geometry.jl:82
┌ Warning: `rotation_between(from::AbstractVector, to::AbstractVector)` is deprecated, use `rotation_between(SVector{3}(from), SVector{3}(to))` instead.
│   caller = settransform!(vis::ArrowVisualizer{Visualizer}, base::Point3{Int64}, vec::Vec3{Float64}; shaft_radius::Float64, max_head_radius::Float64, max_head_length::Float64) at arrow_visualizer.jl:48
└ @ MeshCat ~/git/MeshCat.jl/src/arrow_visualizer.jl:48
┌ Warning: `rotation_between(from::AbstractVector, to::AbstractVector)` is deprecated, use `rotation_between(SVector{3}(from), SVector{3}(to))` instead.
│   caller = settransform!(vis::ArrowVisualizer{Visualizer}, base::Point3{Int64}, vec::Vec3{Int64}; shaft_radius::Float64, max_head_radius::Float64, max_head_length::Float64) at arrow_visualizer.jl:48
└ @ MeshCat ~/git/MeshCat.jl/src/arrow_visualizer.jl:48

Overall, things seem to work pretty well! 🎉 Here's a screenshot of a relatively busy scene using this MeshCat branch (and a develop version of MeshCatMechanisms), running on macOS with Julia 1.9 and on an Electron window:
image

@rdeits
Copy link
Owner Author

rdeits commented May 21, 2023

Thank you--that's extremely helpful!

I tested this on Windows as well and verified that #234 is still fixed on this branch. I'll see if I can fix that IOError after closing the electron window and then merge this.

@rdeits rdeits merged commit b6825ec into master May 21, 2023
@ferrolho ferrolho deleted the rd/http-websockets branch May 21, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants