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

fix gridlines on filled surfaces #27

Merged
merged 1 commit into from
Apr 14, 2022
Merged

fix gridlines on filled surfaces #27

merged 1 commit into from
Apr 14, 2022

Conversation

ChristopherChudzicki
Copy link
Collaborator

Is there a related issue? #25

#25 is mostly about shaded surfaces being too dark, but we also noticed a gridline issue there, too.

What does this PR do?

In #25 we noted that recent versions of mathbox (including znah's ThreeJS r84 fork) have black gridlines. This fixes that issue.

v0.0.5 Gridlines exactly same color as surface, so not visible unless surface is shaded. From the code in 0.0.5 it's clear the intent was to darken the gridlines, but this appears not to have been working in 0.0.5
Screen Shot 2022-04-13 at 6 23 40 PM

v2.1.3 Black gridlines in all cases. Also darker shaded surface; see #26
Screen Shot 2022-04-13 at 6 20 38 PM

This branch Gridlines of proper color; darker surface will be fixed by #26
Screen Shot 2022-04-13 at 6 17 41 PM

@@ -277,13 +277,13 @@ export class Surface extends Primitive {
this.wireColor.copy(color);
if (fill) {
const c = this.wireScratch;
c.setRGB(color.x, color.y, color.z);
c.setRGB(color.r, color.g, color.b);
Copy link
Collaborator

Choose a reason for hiding this comment

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

AH!!! Nice fix :)

Copy link
Collaborator

@sritchie sritchie left a comment

Choose a reason for hiding this comment

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

very nice, this is great.

@sritchie sritchie merged commit b6c1e7c into master Apr 14, 2022
@sritchie sritchie deleted the cc/gridlines branch April 14, 2022 03:54
@sritchie sritchie mentioned this pull request Apr 14, 2022
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.

2 participants