rohitvarkey / ThreeJS.jl

Julia interface to WebGL using Three-js custom elements and Patchwork.jl
https://rohitvarkey.github.io/ThreeJS.jl
Other
56 stars 15 forks source link

Refactored three-js-box to use an observer of multiple properties #39

Closed izaid closed 7 years ago

izaid commented 7 years ago

This PR changes three-js-box so that it uses a single observer on its width, height, and depth properties. I've also cleaned it up a bit, removing an unused property extent and renaming things to match their ThreeJS names.

Both @rohitvarkey and I found that more complex observers, like observers on multiple properties, weren't working before. But apparently they are now. I'd like to slowly migrate things to use observers like that, as it would be much more efficient.

There is a secondary problem here that I have not solved. If we use something like ThreeJS.box from Julia, a new box will be constructed on the JavaScript side every time (say) either the width or the height changes. If we change them both at the same time, we construct the box twice.

That problem probably needs to be fixed on the Julia side. One option would be putting all the parameters in a Dict and serializing that, with the side effect that those parameters could no longer be attributes. Another option would be to encourage the use of scale from ThreeJS instead.

codecov-io commented 7 years ago

Current coverage is 87.09% (diff: 100%)

Merging #39 into master will not change coverage

@@             master        #39   diff @@
==========================================
  Files             3          3          
  Lines            93         93          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits             81         81          
  Misses           12         12          
  Partials          0          0          

Powered by Codecov. Last update 818ea36...8ef2442

rohitvarkey commented 7 years ago

@izaid You can go ahead and merge this with your newfound commit powers. 😄

izaid commented 7 years ago

Done!