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

Using UPERCASE_STYLE for constants? #1152

Closed
mrdoob opened this issue Jan 20, 2012 · 16 comments
Closed

Using UPERCASE_STYLE for constants? #1152

mrdoob opened this issue Jan 20, 2012 · 16 comments

Comments

@mrdoob
Copy link
Owner

mrdoob commented Jan 20, 2012

So @chandlerprall was suggesting having a version variable somewhere in the API that a plugin or something that sits on top can use. He was suggesting THREE.version. The idea sounded good to me but the naming didn't feel right. If it's a constant it should be upercase THREE.VERSION. @chandlerprall then pointed out that maybe should be THREE.Version as that's how we're handling the constants in other places.

That made me think that probably this needs reconsideration. Right now there is no difference between CubeReflectionMapping and RepeatWrapping. Even if the first one is a method and the second a constant. Also, they're poluting the root of the namespace and it makes it hard to know where these constants can be used and also which file to look for (when messing with the code).

So what about changing THREE.CubeReflectionMapping to THREE.Texture.CubeReflectionMapping THREE.RepeatWrapping to THREE.Texture.REPEAT_WRAPPING, ... ?

@alteredq
Copy link
Contributor

If only we could have them naked ...

I often use UPPERCASE constants as globals on the application level, but they look rather ugly with THREE namespace in front of them (THREE.REPEAT_WRAPPING is quite unsightly).

And THREE.Texture.REPEAT_WRAPPING is too long for my taste, already THREE.RepeatWrapping feels quite long.

About CubeReflectionMapping, I actually never got used to it being a function, it still feels wrong.

Good point about not knowing where to look for constants, it bugs me too. Maybe we should keep them all in a single file?

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 21, 2012

From all these maybe THREE.REPEAT_WRAPPING is the better looking? THREE.Texture.REPEAT_WRAPPING is indeed ugly and long and a "How did we get here?" kind of case.

CubeReflectionMapping being functions. Well, the idea of these is that they would have extra information. Like the amount of refraction for CubeRefractionMapping and the UV set for UVMapping. We'll get there :)

Also, there is this thing... should it be THREE.VERSION or THREE.REVISION?

@chandlerprall
Copy link
Contributor

I think THREE.VERSION fits better, it feels more accurate. Just my 2¢ :)

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 21, 2012

I'll have to change the "r" in r48 to v48. In a bunch of places...

@chandlerprall
Copy link
Contributor

True. Version numbers do tend to stay low while revision numbers grow until a versioned release is ready. Moving that to a version number would be 1.0.48 or 0.0.48 which doesn't really fit the library. Perhaps THREE.REVISION is more appropriate here.

@jeromeetienne
Copy link
Contributor

maybe with semver ? http://semver.org/

it tries to formalize how to use version. close to linux version system

it is from mojomdo a github guy.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 23, 2012

That's way too complex for my little brain... :/

@chandlerprall
Copy link
Contributor

Since three has historically been marked with revisions and not versions, THREE.REVISION keeps it simple until there's reason to change it.

@mrdoob
Copy link
Owner Author

mrdoob commented Jan 23, 2012

Sounds good. THREE.REVISION it is then. Changed! :)

@jeromeetienne
Copy link
Contributor

cool :)

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 14, 2012

Good point about not knowing where to look for constants, it bugs me too. Maybe we should keep them all in a single file?

We can do like it's done in three.dart. All the constants would sit in the main Three.js file.
https://github.com/robsilv/three.dart/blob/master/src/Three.dart

@alteredq
Copy link
Contributor

Yup, could be.

Sidenote: I'm really curious about Dart performance, it would be very interesting to have some of our stress tests ported to Three.dart and compare them with JS ones.

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 14, 2012

Well, I have the feeling is going to take a while before we can compare stress tests... There is quite a bit to port ;)

Also, I think Dart will be interesting whenever a browser can execute it natively. Right now files like this don't look too promising imho.

Still, I'm also curious to see the performance this compiled javascript can get.

@alteredq
Copy link
Contributor

Oh, it's not native yet? I somehow got impression it was already native in some version of Chrome [1].

Then no, compiled to JS it's practically useless. Even about native I don't have very high expectations, our bottlenecks are mostly JS to WebGL boundaries not raw JS performance. Could make a difference for stuff like physics though.

Edit: [1] here is the best I could find (Dartium, Chrome with native DartVM, prebuilt Linux and Mac binaries, seems nobody managed to build Windows executable yet, also not sure if WebGL works there at all):

http://financecoding.wordpress.com/2012/01/25/dart-updated-dartium-builds-with-breakpoint-support-for-linux-and-mac/

@mrdoob
Copy link
Owner Author

mrdoob commented Feb 16, 2012

@alteredq more official: http://www.dartlang.org/dartium/

@dubejf dubejf mentioned this issue Jun 30, 2015
@dubejf
Copy link
Contributor

dubejf commented Jul 1, 2015

✅ THREE.REVISION is already implemented

As for the rest:

  • Changing constants to UPPERCASE would break at lot of code for little benefit
  • three.js kinda use semver, as all releases contain breaking changes (not sure if we are at 72.0.0 or 0.72.0 though)
  • I think that the momentum for dart has dissipated.

@dubejf dubejf closed this as completed Jul 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants