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

#1010 Colormaker-constructor #1011

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

ppillot
Copy link
Collaborator

@ppillot ppillot commented Jan 8, 2024

This PR fixes issue #1010 where the syntax for registering a ColorMaker does not generate valid JS.

After bissection, I've found out that this issue has been introduced with the recent changes to the build process (PR #993). It's possible that the transpilation + bundling results in invalid JS code (?).
The fix was already done in the update-three,js ongoing branch, and had been cherry-picked from there.

Previous implementation of Colormaker was probably relying
on JS prototypal inheritance.
Colormaker cannot be called like a function anymore.

(cherry picked from commit 0899733)
@ppillot ppillot marked this pull request as ready for review January 8, 2024 03:07
Copy link
Collaborator

@fredludlow fredludlow left a comment

Choose a reason for hiding this comment

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

That is a little strange, and surprising why the existing classical (explicit prototype) method fails, but switching to the class syntax reads more clearly anyway.

I take it this works with:

a. Regular built-in colorschemes (uniform etc)
b. Colorschemes added by a user which implement atomColor etc
c. Selection colorschemes?

If so then all looks good to me.

@ppillot
Copy link
Collaborator Author

ppillot commented Jan 8, 2024

@fredludlow I've made some further tests and, noticed that one of my projects, it breaks the Typescrip checks. If I ignore the checks, the code works well though.

image

I'll investigate further.

One type of argument was missing, as `scheme` can have 2 shapes (either a class derived from Colormaker or a function that redefines the Colormaker prototype on a derived class).
It was necessary to add type predicates to correctly infer the type of `scheme` within the addScheme method.
@ppillot
Copy link
Collaborator Author

ppillot commented Jan 9, 2024

Issue resolved: it was a missing typescript definition for the argument of the addScheme method.
I've checked on a personal project and in every "Color" example of the NGL webapp.

@ppillot ppillot merged commit ddea04f into nglviewer:master Jan 9, 2024
2 checks passed
@ppillot ppillot deleted the colormaker-constructor branch January 9, 2024 03:24
@ppillot
Copy link
Collaborator Author

ppillot commented Jan 9, 2024

That is a little strange, and surprising why the existing classical (explicit prototype) method fails, but switching to the class syntax reads more clearly anyway.

I am wondering if that's not an issue with terser, which can turn function declarations in more compact arrow functions. It's supposed to avoid doing so when there a this in the function body...

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.

None yet

2 participants