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

Render slices on 3d object #95

Merged
merged 7 commits into from
Mar 24, 2023
Merged

Render slices on 3d object #95

merged 7 commits into from
Mar 24, 2023

Conversation

muddv
Copy link
Contributor

@muddv muddv commented Mar 23, 2023

Related to #90
Currently 3d object look like this:
image
image

And here it is in action:
https://user-images.githubusercontent.com/73133951/227347489-7bea7f0b-9724-4c7c-aefa-5f993c2efe07.mp4

@ai
Copy link
Member

ai commented Mar 23, 2023

I sent it to our designer. For me it looks awesome.

@ai
Copy link
Member

ai commented Mar 24, 2023

Looks awesome for both of us. Tomorrow will do a review if we are ready.

lib/model.ts Outdated
if (!fullControl) {
updateSelectors(selectorL, selectorC, selectorH, current.get())
}
current.listen(value => {
Copy link
Member

Choose a reason for hiding this comment

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

Why we need to listen current for cull-screen model (we do not need to draw lines on fullscreen)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we are in fullscreen current would not be updating anyway, so it does not influence anything. Shaders are reset to default value, so selectors are not visible in fullscreen

Copy link
Member

Choose a reason for hiding this comment

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

But why we need this line in full-screen? Can we move it to if (!fullControl)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I made the change, also automatically fixed some formatting

@romashamin
Copy link
Member

Wow! That looks awesome! Thank you, @muddv.

Isn’t it too late to make lines thinner? I think half of the current stroke would be great.

@muddv
Copy link
Contributor Author

muddv commented Mar 24, 2023

Wow! That looks awesome! Thank you, @muddv.

Isn’t it too late to make lines thinner? I think half of the current stroke would be great.

Thank you!

Making lines thinner is a quick change, here is how it would look like at half of current width:
image
What do you think?

@romashamin
Copy link
Member

@muddv thank you!

This width looks perfect 🔥

There’s nothing else from my side! cc @ai

@ai ai merged commit 11d5fe7 into evilmartians:main Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants