Skip to content

Finalist - Improve rendering of lines in 3D (against disappearing in certain angles) issue #691 #3163

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 15 commits into from
Oct 26, 2018
Merged

Finalist - Improve rendering of lines in 3D (against disappearing in certain angles) issue #691 #3163

merged 15 commits into from
Oct 26, 2018

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 26, 2018

This PR is the continuation of PR #3128 where the master is fetched and conflicts are resolved to facilitate the merge process.
Previously the angles computed in the gl-plot3d module necessary to match rectangle shapes towards camera point were not accurate. Therefore 2D representations of 3D lines were not oriented properly in various perspective angles. Disappearing and tininess impacts resulted from the bug could be clearly observed namely when the thicknesses of lines using scatter3d were increased. Please refer to the issue #691 for more information.
This PR improves rendering of lines in 3D scenes by performing proper 3D/2D calculations within the webgl vertex shader of the module.
@alexcjohnson
@etpinard

@archmoj archmoj added bug something broken status: reviewable labels Oct 26, 2018
@etpinard
Copy link
Contributor

Nice work @archmoj

So if I understand correctly, issue #691 was visible in the plot_types mock

peek 2018-10-26 09-26

and is now fixed?

@etpinard etpinard mentioned this pull request Oct 26, 2018
Closed
@archmoj
Copy link
Contributor Author

archmoj commented Oct 26, 2018

Yes it was visible. And it should be fixed now.
FYI @etpinard I also created a new mock in this PR to better highlight this issue namely with 3D lines with greater thicknesses.

@etpinard
Copy link
Contributor

Great! Well done 💃

@etpinard
Copy link
Contributor

etpinard commented Oct 26, 2018 •
edited
Loading

... after you publish a new gl-line3d patch version of course.

@archmoj
Copy link
Contributor Author

archmoj commented Oct 26, 2018

Could you please give me a confirmation for the PR on gl-vis too?

@archmoj
Copy link
Contributor Author

archmoj commented Oct 26, 2018

Hey wait,
Still need to address @alexcjohnson comment concerning gl3d_set-ranges here
https://github.com/plotly/plotly.js/pull/3163/files?short_path=17cbbc9#diff-17cbbc97019f5b261179c860f0dcef28

@archmoj
Copy link
Contributor Author

archmoj commented Oct 26, 2018

With the latest changes the difference between old image baselines (on current master) & new image baselines is much reduced. The gl3d_set-ranges is actually removed from the list of changed files. So we should be good to continue with merging the PR on gl-vis then this branch. What do you think @etpinard ? Would you give me two dancers to merge one after the other?

@etpinard
Copy link
Contributor

new image baselines is much reduced.
The gl3d_set-ranges is actually removed from the list of changed files

That's great news. I say 💃 (unless @alexcjohnson find other suspicious things from the diff images)

@alexcjohnson
Copy link
Collaborator

💃

@archmoj archmoj merged commit aea6b8d into master Oct 26, 2018
@archmoj archmoj deleted the issue-691_fetch-master branch October 26, 2018 20:09
@etpinard etpinard mentioned this pull request Oct 29, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug something broken
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@archmoj @etpinard @alexcjohnson