Skip to content

Conversation

@yushijinhun
Copy link
Contributor

@yushijinhun yushijinhun commented Jan 31, 2020

Code is converted using yushijinhun/convert-shader with some manual fixes.

  • /* glsl */ prefix is added, which should enable syntax highlighting (see Add syntax highlighting for glsl.js files #15473).
  • Comments outside string literals are moved into the string template. For example:
    [
        // this is comment
        "    this is code"
    ].join( "\n" )
    now becomes
    /* glsl */`
        // this is comment
        this is code
    `

@looeee
Copy link
Collaborator

looeee commented Feb 1, 2020

Related: #15506

Support for template literals (>93%, basically everywhere except IE11).

As far as I know we already switched to template strings in src/. Seems like we should make the change in examples too. Or was there something holding this back previously?

Copy link
Contributor Author

@yushijinhun yushijinhun left a comment

Choose a reason for hiding this comment

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

d40fa09 is cherry-picked from b1e8bf7, see discussion here

@yushijinhun yushijinhun requested a review from mrdoob February 2, 2020 07:20
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 10, 2020

Since the examples all use modules now, I think it's fine if the code starts using string templates.

@looeee AFAIK, the browser support was the main reason why #15516 was never merged. However, the situation has changed in the meanwhile and doing this change seems fine now.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 10, 2020

@yushijinhun Do you mind resolving the merge conflicts? I'd like to test the PR.

https://raw.githack.com/yushijinhun/three.js/glsl/examples/index.html

@Mugen87 Mugen87 mentioned this pull request Mar 11, 2020
@mrdoob
Copy link
Owner

mrdoob commented Mar 11, 2020

I don't think it's a good idea to implement template literals in examples/js, these files are supposed to work on IE11 still.

@looeee
Copy link
Collaborator

looeee commented Mar 12, 2020

these files are supposed to work on IE11 still.

Ah, fair point. We should wait until examples/js has been removed then.

@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2020

Yes... Sorry, closing this for now.

@mrdoob mrdoob closed this Mar 12, 2020
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 12, 2020

@mrdoob Do you already have a point of time in mind until IE11 support will be stopped?

@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2020

I was actually thinking that we should add one legacy example that still works in IE11 (and point to it in the examples when js modules are not available).

It's not only IE11, it's also KaiOS (which is stuck in an old version of Firefox):
https://twitter.com/ChristianWaadt/status/1230222217872650245

Today there were good news on that front though:
https://www.kaiostech.com/press/kaios-technologies-and-mozilla-partner-to-enable-a-healthy-mobile-internet-for-everyone/

@looeee
Copy link
Collaborator

looeee commented Mar 13, 2020

I was actually thinking that we should add one legacy example that still works in IE11

We could (instead, or also) add a docs page "How to support outdated browsers" that gives a brief overview of bundling and transpiling.

@marcofugaro
Copy link
Contributor

I agree with @looeee, here is an example of how the create-react-app team is doing it: https://github.com/facebook/create-react-app/tree/master/packages/react-app-polyfill

Also we should move src/polyfill.js into examples/jsm/polyfill.js and show its usage in the example/docs.
It looks to me that every polyfill we have is there only for IE11.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 13, 2020

We could (instead, or also) add a docs page "How to support outdated browsers" that gives a brief overview of bundling and transpiling.

I like this idea since it allows the project to use more modern language features and APIs and still shows users how to produce a build than runs in (legacy) environments.

@yushijinhun
Copy link
Contributor Author

@Mugen87 I've resolved the conflicts. You can still test it here if you want to.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 13, 2020

Thanks but this obviously makes no sense now since the PR isn't merged anyway. Sorry.

@yushijinhun
Copy link
Contributor Author

yushijinhun commented Mar 13, 2020

I can create a new PR when it's okay to introduce string templates, since it's definitely going to happen.

BTW, if you are going to do transpiling, I think it's a good time to minify all GLSL code (not only those in .glsl.js files, but strings prefixed by /* glsl */ in all .js files)

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.

6 participants