search

elm-community / elm-webgl

Moved to elm-explorations/webgl
http://package.elm-lang.org/packages/elm-explorations/webgl/latest
BSD 3-Clause "New" or "Revised" License
95 stars 22 forks source link

Added clear color function. #9

Closed mchav closed 8 years ago

mchav commented 8 years ago

Not the best implementation but I needed a way to set a clear color and I think that sort of functionality is generally useful.

mgold commented 8 years ago

@ChavXO This doesn't even compile. You need to add a | ClearColor (Float, Float, Float, Float) case to FunctionCall, around line 310. Doing so is a major version bump, which conflicts with #6.

Additionally, your implementation sets the clearColor as a global mutable variable, which will cause all kinds of "action at a distance".

We cannot accept this PR at this time.

mchav commented 8 years ago

@mgold I figured the global variable was a problem. How would you suggest I proceed?

It was suggested by John :) https://groups.google.com/forum/?nomobile=true#!topic/elm-discuss/DZZ41fGxalo

mgold commented 8 years ago

I saw that after I closed, sorry! If John approves the change then that would allow us to make a version bump. As for the global, you'll need to attach it to...I'm not sure if it would be a Renderable, a Drawable, or something else, but the thing that's getting its clear color set. And you'd need to update that value in an immutable way.

mchav commented 8 years ago

@mgold. Thanks. I'm relatively new to the Elm community so I wasn't caught up with the the issues surrounding native libraries and Evan's message about approving version bumps. Caught up now.

Turns out I didn't even need a global. It suffices to return the function. Should I open another pull request?

mgold commented 8 years ago

Sure, take another stab :) (You could force-push to this branch or something, but let's start fresh.)