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

Break up constants into section specific namespaces #5

Open
rm-hull opened this issue Mar 26, 2014 · 6 comments
Open

Break up constants into section specific namespaces #5

rm-hull opened this issue Mar 26, 2014 · 6 comments

Comments

@rm-hull
Copy link
Collaborator

rm-hull commented Mar 26, 2014

Was wondering whether it might be an idea make better use of namespaces to separate the different groups of constants (almost treating them as pseudo-enums): this might work by creating a src/cljs-webgl/constants folder, and then break up all the constants into smaller files, eg. src/cljs-webgl/constants/begin-mode.cljs would contain:

;; BeginMode
(ns cljs-webgl.constants.begin-mode)

(def points 0x0000)
(def lines 0x0001)
(def line-loop 0x0002)
(def line-strip 0x0003)
(def triangles 0x0004)
(def triangle-strip 0x0005)
(def triangle-fan 0x0006)

src/cljs-webgl/constants/blending-factor-dest.cljs would contain:

;; BlendingFactorDest
(ns cljs-webgl.constants.blending-factor-dest)

(def zero 0)
(def one 1)
(def src-color 0x0300)
(def one-minus-src-color 0x0301)
(def src-alpha 0x0302)
(def one-minus-src-alpha 0x0303)
(def dst-alpha 0x0304)
(def one-minus-dst-alpha 0x0305)

etc.

Then referencing:

(ns blah
  (:require
    [cljs-webgl.constants.begin-mode :as begin-mode]
    ...))

...

(draw! 
  gl-context
  :shader shader-program
  :draw-mode begin-mode/triangles
  ...)
@asakeron
Copy link
Owner

Yes, that sounds like a good ideia.

I've been thinking about replacing the constants with keywords for some time now as it would require some sort of lookup function and this could work as a way of self documenting the accepted values.

I believe your idea would suffice, though. What do you think?

@rm-hull
Copy link
Collaborator Author

rm-hull commented Mar 26, 2014

I did wonder about keywords, but there will be a cost associated with the lookup function - whereas this'll come for free using namespaces and defs (I'm presuming) as the constants will get replaced at compile time. The only downside of using namespaces is that there will be a proliferation of source files (I dont think you can have many namespaces in one file?)

@asakeron
Copy link
Owner

I don't see a larger number of files as a big problem, and the fact that each of them defines related constants is a bonus for readability. So yes, lets do it! 😃

@asakeron
Copy link
Owner

Fixed in 658570d.

@rm-hull
Copy link
Collaborator Author

rm-hull commented Apr 4, 2014

Nice one 👍

I fixed a typo - s/texure/texture/ in one of the cljs file

Also, I wonder if namespaces texture-mag-filter and texture-min-filter should be collapsed into a single namespace texture-filter ? what do you think?

Reason I ask is I've currently got a call out to (not checked this in yet):

(init-texture gl
  :image img
  :texture tex2
  :parameters {texture-parameter-name/texture-mag-filter texture-mag-filter/linear
               texture-parameter-name/texture-min-filter texture-mag-filter/linear})

seems a bit odd to have a key texture-parameter-name/texture-min-filter with value texture-mag-filter/linear

Collapsing to a single namespace this becomes:

(init-texture gl
  :image img
  :texture tex2
  :parameters {texture-parameter-name/texture-mag-filter texture-filter/linear
               texture-parameter-name/texture-min-filter texture-filter/linear})

Other option might be to replicate linear & nearest:

(ns cljs-webgl.constants.texture-min-filter)

(def nearest 0x2600)
(def linear  0x2601)

(def nearest-mipmap-nearest 0x2700)
(def linear-mipmap-nearest  0x2701)
(def nearest-mipmap-linear  0x2702)
(def linear-mipmap-linear   0x2703)

and then this looks like:

(init-texture gl
  :image img
  :texture tex2
  :parameters {texture-parameter-name/texture-mag-filter texture-mag-filter/linear
               texture-parameter-name/texture-min-filter texture-min-filter/linear})

@rm-hull rm-hull reopened this Apr 4, 2014
@asakeron
Copy link
Owner

asakeron commented Apr 5, 2014

@rm-hull I merged texture-min-filter and texture-mag-filter into a single namespace called texture-filter in 0cbd38e as you suggested. There are probably other namespaces that can be improved, so I will keep this issue open.

PS: I used the list of values in the WebGL Specification as a reference to break the constants into namespaces.

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

2 participants