h3n3/cadhub
1
0
Fork 0
Code Issues 35 Pull Requests Packages Projects Releases Wiki Activity

initial jscad integration #428

Merged
hrgdavor merged 15 commits from kurt/411-demo-branch into main 2021-08-01 11:35:56 +02:00
Conversation 16 Commits 15 Files Changed 10 +843 -19
hrgdavor commented 2021-07-31 21:06:45 +02:00 (Migrated from github.com)
Copy Link

Please review, and any feedback on coding style is welcome.

Also preview refresh uses direct access to threejs using useThree() and adds geometries to the scene and also removes and makes sure they are also diposed. I works so it is not critical to get it fixed now. Sbdy with more experience with react could maybe do it without direct access.

Please review, and any feedback on coding style is welcome. Also preview refresh uses direct access to threejs using useThree() and adds geometries to the scene and also removes and makes sure they are also diposed. I works so it is not critical to get it fixed now. Sbdy with more experience with react could maybe do it without direct access.
👍 1 👀 1
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 10:31:46 +02:00
app/web/public/demo-worker.js
@@ -0,0 +1,625 @@
(function(f) {
Irev-Dev (Migrated from github.com) commented 2021-08-01 10:31:46 +02:00
Copy Link

I understand why you've done it, having this file just hanging in the public folder is not something I'd like to have long term, mostly because unlike the rest of the repo this isn't transpiled so suddenly we have to be concious of old browsers just for this one file.

I understand why you've done it, having this file just hanging in the public folder is not something I'd like to have long term, mostly because unlike the rest of the repo this isn't transpiled so suddenly we have to be concious of old browsers just for this one file.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 10:37:05 +02:00
app/web/public/demo-worker.js
@@ -0,0 +131,4 @@
let workerBaseURI
function require(url){
Irev-Dev (Migrated from github.com) commented 2021-08-01 10:37:05 +02:00
Copy Link

👌 out of curiosity are we currently locked into the old node require syntax only? Wondering if you have ideas for import syntax?

👌 out of curiosity are we currently locked into the old node `require` syntax only? Wondering if you have ideas for `import` syntax?
hrgdavor (Migrated from github.com) reviewed 2021-08-01 10:38:28 +02:00
app/web/public/demo-worker.js
@@ -0,0 +1,625 @@
(function(f) {
hrgdavor (Migrated from github.com) commented 2021-08-01 10:38:28 +02:00
Copy Link

There are 2 ways to run a worker.

  1. js file
  2. generated code in a blob

option 2. would enable bundling the code together with rest of the app and can be done in two ways

  • paste the whole worker code into a string
  • wrap the worker code in a function and use funtion.toString() to extract the code later

worker is still heavily under construction, and currently this way it is easier to debug for me. After it is further improved we will look to integrate it instead of having it like this in public folder.

There are 2 ways to run a worker. 1. js file 2. generated code in a blob option 2. would enable bundling the code together with rest of the app and can be done in two ways - paste the whole worker code into a string - wrap the worker code in a function and use funtion.toString() to extract the code later worker is still heavily under construction, and currently this way it is easier to debug for me. After it is further improved we will look to integrate it instead of having it like this in public folder.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 10:39:39 +02:00
app/web/public/demo-worker.js
@@ -0,0 +184,4 @@
let pointerDown = false
const moveHandler = (ev) => {
Irev-Dev (Migrated from github.com) commented 2021-08-01 10:39:39 +02:00
Copy Link

I don't quiet follow how these handlers fit in? especially since this runs in the worker. Could you explain it a little?

I don't quiet follow how these handlers fit in? especially since this runs in the worker. Could you explain it a little?
hrgdavor (Migrated from github.com) reviewed 2021-08-01 10:41:07 +02:00
app/web/public/demo-worker.js
@@ -0,0 +131,4 @@
let workerBaseURI
function require(url){
hrgdavor (Migrated from github.com) commented 2021-08-01 10:41:07 +02:00
Copy Link

it is just a piece of code I was able to hack-up that works. Like that hack useThree for react-fiber. The worker needs more refinement for sure.

it is just a piece of code I was able to hack-up that works. Like that hack `useThree` for react-fiber. The worker needs more refinement for sure.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 10:44:58 +02:00
app/web/src/helpers/cadPackages/CSGToBuffers.js
Irev-Dev (Migrated from github.com) commented 2021-08-01 10:44:58 +02:00
Copy Link

I'm a bit confused about this file, both in that you're exporting a function here but I can't figure out where it's imported and used? Also I noticed that a lot of the functions in this file are duplicated in the demo worker as well, what's going on there?

I'm a bit confused about this file, both in that you're exporting a function here but I can't figure out where it's imported and used? Also I noticed that a lot of the functions in this file are duplicated in the demo worker as well, what's going on there?
hrgdavor (Migrated from github.com) reviewed 2021-08-01 10:46:30 +02:00
app/web/public/demo-worker.js
@@ -0,0 +184,4 @@
let pointerDown = false
const moveHandler = (ev) => {
hrgdavor (Migrated from github.com) commented 2021-08-01 10:46:30 +02:00
Copy Link

it is code when canvas is used directly
The worker has 3 parts:

  • renderer
  • orbit controls
  • script eval

I was not sure if we would go in direction of direct canvass access. for now, this integration only "script eval" part is needed.

I curently copy/pasta the worker from my other prototype. After I refine the prototype worker, I will have a version that only has "script eval" part.

it is code when canvas is used directly The worker has 3 parts: - renderer - orbit controls - script eval I was not sure if we would go in direction of direct canvass access. for now, this integration only "script eval" part is needed. I curently copy/pasta the worker from my other prototype. After I refine the prototype worker, I will have a version that only has "script eval" part.
hrgdavor (Migrated from github.com) reviewed 2021-08-01 10:48:21 +02:00
app/web/src/helpers/cadPackages/CSGToBuffers.js
hrgdavor (Migrated from github.com) commented 2021-08-01 10:48:21 +02:00
Copy Link

it is a leftower from work in progress. I have deleted it

it is a leftower from work in progress. I have deleted it
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 11:04:09 +02:00
app/web/src/helpers/cadPackages/jsCadController.ts
@@ -0,0 +127,4 @@
})
const waitResult = new Promise((resolve) => {
resolveReference = resolve
Irev-Dev (Migrated from github.com) commented 2021-08-01 11:04:09 +02:00
Copy Link

hmmm I might have a bit of think of how we might be able to change this, and a few other bits related to turning a worker message into a promise. assigning the resolve function to an upper scope var smells a bit off to me.

hmmm I might have a bit of think of how we might be able to change this, and a few other bits related to turning a worker message into a promise. assigning the resolve function to an upper scope var smells a bit off to me.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 11:18:20 +02:00
Irev-Dev (Migrated from github.com) left a comment
Copy Link

Nice work @hrgdavor.
So good to have another integration, three feels like a milestone somehow.

My thoughts are that there are a few things I'd like to see changed but think it makes sense to merge as is because it would be good to demo to the JSCAD team before putting too much effort into this (as we talked about previously). I think at the point where we are looking to allow users to save jscad projects, at that point I'll want to see a couple things changed.

I've commented on them above but's really just moving the worker out of public and probably something related to the promise from the worker message.

Also I'm curious if you would mind if I refactored some of the jsCadController to make it a little more react-three-fiber-centric?

In regards to the change I made in IdeViewer I got pretty stuck on that, I was certain we should have been able to render the result directly, but I found it would flash up and then disappear. Doing some logging revealed that the children would disappear after the first render in the Group, I couldn't figure out where but I guessing something must have been mutating that object, and that's like the golden rule for react, not to mutate. Anyway glad I was able to sort it out.

Nice work @hrgdavor. So good to have another integration, three feels like a milestone somehow. My thoughts are that there are a few things I'd like to see changed but think it makes sense to merge as is because it would be good to demo to the JSCAD team before putting too much effort into this (as we talked about previously). I think at the point where we are looking to allow users to save jscad projects, at that point I'll want to see a couple things changed. I've commented on them above but's really just moving the worker out of public and probably something related to the promise from the worker message. Also I'm curious if you would mind if I refactored some of the `jsCadController` to make it a little more `react-three-fiber`-centric? In regards to the change I made in `IdeViewer` I got pretty stuck on that, I was certain we should have been able to render the result directly, but I found it would flash up and then disappear. Doing some logging revealed that the children would disappear after the first render in the `Group`, I couldn't figure out where but I guessing something must have been mutating that object, and that's like the golden rule for react, not to mutate. Anyway glad I was able to sort it out.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 11:19:06 +02:00
app/web/public/demo-worker.js
@@ -0,0 +184,4 @@
let pointerDown = false
const moveHandler = (ev) => {
Irev-Dev (Migrated from github.com) commented 2021-08-01 11:19:06 +02:00
Copy Link

okay yup, cool.

okay yup, cool.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 11:19:32 +02:00
app/web/public/demo-worker.js
@@ -0,0 +131,4 @@
let workerBaseURI
function require(url){
Irev-Dev (Migrated from github.com) commented 2021-08-01 11:19:32 +02:00
Copy Link

yup, sweet.

yup, sweet.
hrgdavor (Migrated from github.com) reviewed 2021-08-01 11:20:51 +02:00
app/web/src/helpers/cadPackages/jsCadController.ts
@@ -0,0 +127,4 @@
})
const waitResult = new Promise((resolve) => {
resolveReference = resolve
hrgdavor (Migrated from github.com) commented 2021-08-01 11:20:51 +02:00
Copy Link

I was thinking that I need to stop pervious job if next one comes before first one is finished. hence the upper scope.

I was thinking that I need to stop pervious job if next one comes before first one is finished. hence the upper scope.
Irev-Dev (Migrated from github.com) reviewed 2021-08-01 11:25:37 +02:00
app/web/public/demo-worker.js
@@ -0,0 +1,625 @@
(function(f) {
Irev-Dev (Migrated from github.com) commented 2021-08-01 11:25:37 +02:00
Copy Link

Fair enough.

Once you're happy with it/it's settled down a little, I'd be happy to work this into the bundle. I did something similar in another webpack project
https://github.com/zalo/CascadeStudio/pull/58/files#diff-d4a06c27ddf0c24f2ba6029bffc9b20062214e07d6ee72822997fb6941b2534d

Fair enough. Once you're happy with it/it's settled down a little, I'd be happy to work this into the bundle. I did something similar in another webpack project https://github.com/zalo/CascadeStudio/pull/58/files#diff-d4a06c27ddf0c24f2ba6029bffc9b20062214e07d6ee72822997fb6941b2534d
hrgdavor commented 2021-08-01 11:27:59 +02:00 (Migrated from github.com)
Copy Link

Also I'm curious if you would mind if I refactored some of the jsCadController to make it a little more react-three-fiber-centric?

I am glad you did that, I tried few things but could not exactly pinpoint the issue. I like to make things work first before looking for a better solution :)

I agree with mentioned changes to worker, it is a work in progress

Code/experience here could be extracted into a small lib that does jscad -> react-fiber integration for others interested.

> Also I'm curious if you would mind if I refactored some of the jsCadController to make it a little more react-three-fiber-centric? I am glad you did that, I tried few things but could not exactly pinpoint the issue. I like to make things work first before looking for a better solution :) I agree with mentioned changes to worker, it is a work in progress Code/experience here could be extracted into a small lib that does jscad -> react-fiber integration for others interested.
👍 1
Irev-Dev (Migrated from github.com) approved these changes 2021-08-01 11:35:39 +02:00
Sign in to join this conversation.
Reviewers
No Reviewers
hrgdavor
Irev-Dev
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
h3n3
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: h3n3/cadhub#428
Delete Branch "kurt/411-demo-branch"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?