search

LCSR-SICKKIDS / volumetric_drilling

Virtual reality for synergistic surgical training and data generation. (MICCAI 2021 AE-CAI Best Paper)
21 stars 20 forks source link

Minor fix for python scripts #8

Closed mli0603 closed 2 years ago

mli0603 commented 2 years ago

Hi @adnanmunawar

My fixes for python scripts are pretty minor. But it looks like your commits from Feb 17th have not been merged into master. Do you mind checking if this is intended?

mli0603 commented 2 years ago

It looks like commit 42588ebe746ae9f68fdd64f1c5a99ed766c890c9 breaks the logic for changing drill size.

adnanmunawar commented 2 years ago

Hi Max, thanks for reminding me. Yes, I have not merged the most recent changes from the master branch of my fork. In fact, I created another branch called enhanced-rendering with further updates that I have yet to create a PR for. I should hopefully get some time to get to it later this week.

mli0603 commented 2 years ago

Hi @adnanmunawar

I fixed the data recording script to log volume pose. One thing I failed to fix after changing the camera parameter is the light location. I couldn't find a configuration so it looks like your version. Do you mind taking a look? Thanks!

adnanmunawar commented 2 years ago

Hi Max, thanks for quickly updating the PR. Regarding the light location, do you mean that you are unable to record its pose in the data_recorded script or that you can't change its location such that it affects the scene lighting?

mli0603 commented 2 years ago

Sorry for the confusion. I meant the look of the scene changed slightly after I move the camera and volume. I tried to change the light location so the look of the scene is the same as before. But I don't think I fully recovered it. Here is a sample image.

Screenshot from 2022-04-27 09-08-10

However, if you think this is close enough, this is may not a problem :)

adnanmunawar commented 2 years ago

Oh I see. Setting the position of light1 to be location: {x: 0.0, y: 2.5, z: 2.0} should replicate the previous lighting. Anyway, we may use MatCap so the light placement would become irrelevant later.

P.S. I tried pushing this as a commit to your fork so the PR gets updated but I don't have the rights :)

mli0603 commented 2 years ago

I hope I just fixed the access issue. I’ll let you make the change so we can test if it’s actually fixed :)

adnanmunawar commented 2 years ago

Great :D. I am merging this now.