Permalink
Browse files

Refactored materials code.

  • Loading branch information...
mrdoob committed Aug 6, 2012
1 parent 4b962fa commit 4845632e1596812352a33abb2375d289ea8ddc69
View

Large diffs are not rendered by default.

Oops, something went wrong.

Some generated files are not rendered by default. Learn more.

Oops, something went wrong.
Oops, something went wrong.

31 comments on commit 4845632

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Hmmm, not sure it's a good idea to have base Material class needing to know about properties existing in subclasses.

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

If you don't want that, then you could check the with 2 instanceof's what type the original was and base your decision on that.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Hmmm, not sure it's a good idea to have base Material class needing to know about properties existing in subclasses.

Uhm, I can't see any problem with it myself?

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Uhm, I can't see any problem with it myself?

That's surprising, you like clean designs ;)

Problem is this creates dependency, subclasses are not anymore self-contained. If you do change in subclass, even if logically it doesn't have effect on anything else beyond subclass, you will still need to change, or at least check if change is needed, in superclass.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Uhm, I can't see any problem with it myself?

That's surprising, you like clean designs ;)

Haha, well, the code is much more cleaner now. The .clone() methods become logicless and we get a .setValues() method in all the materials for the people that like the .setValues( { property: value, ... } ) pattern ;)

Problem is this creates dependency, subclasses are not anymore self-contained. If you do change in subclass, even if logically it doesn't have effect on anything else beyond subclass, you will still need to change, or at least check if change is needed, in superclass.

Uhm, what would be an example of that?

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

It was either doing this magic:

4845632#L6R40

or this one:

4b962fa#L0R7

The first one is more or less easy to see where it's coming from, the second one it's hard to know where it's coming from and what would break if that line was removed.

@gero3

This comment has been minimized.

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Haha, well, the code is much more cleaner now.

Well, code looks cleaner but overall design is less sound than it was before. Coupling is bad, it's coupling which what makes complex systems difficult.

Uhm, what would be an example of that?

I don't know, let's say we decide to rename wrapRGB to something else, like wrapColorWeights.

Before you just needed to take care of fixing classes that are affected - MeshLambertMaterial and MeshPhongMaterial.

Now there is extra magic step - you need to remember that also Material class has to be changed.

And this holds for potentially all future material subclasses - if they have some "special needs" properties, instead of putting necessary cloning logic in their corresponding files, we will need to keep handling these in base Material class.

I know it's a bad design because I already made one like this ;) That's what sucks in SceneUtils.cloneObject:

https://github.com/mrdoob/three.js/blob/master/src/extras/SceneUtils.js#L45

The first one is more or less easy to see where it's coming from, the second one it's hard to know where it's coming from and what would break if that line was removed.

I'm afraid I don't follow. I also didn't like that Color constructor thing.

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Actually you could do this magic too

Yup, that's much better, this doesn't have dependency.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Actually you could do this magic too

Yup, that's much better, this doesn't have dependency.

Indeed!

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Well, we still have a tiny problem... fb22058
But I guess it's ok?

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

another problem could be needsUpdate.

I guess we could set needsUpdate always after executing setValues.

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Hmmm, there may be also problems with internal properties that are generated in runtime like material.program.

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

Those get updated after needsUpdate, so that shouldn't be a problem.

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

No, this will actually make more problems :S.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Yeah, seems like it'll be better to go back on being dumb, and create specific clone() for each material.

At least now the constructors are now readable :)

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

The problem is that I don't know how to deal with super.clone()... b973152

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

easy

 super.clone() = THREE.material.prototype.clone.apply(this)
@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Uhm, this is what I've tried but I'm not too sure... aadb1d7
Coding wise, it would be nice to have Material.clone() working on it's own, even if it's never used on it's own... out of ideas...

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

I would say make it an optional argument

material = material || new THREE.material()
@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Maybe we could keep generic thing but then whitelist per-material-subclass parameters which do get copied values.

@gero3

This comment has been minimized.

Contributor

gero3 replied Aug 6, 2012

An extra property then with an array of properties that are allowed to be copied. It wouldn't be a bad idea.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

I would say make it an optional argument

material = material || new THREE.material()

Thanks! Implemented. 2c86fbb

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Materials in current dev are badly broken. I can try to fix it, just I need less moving target, there is too much chaos going on today.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

What is it broken? As far as I'm aware is mainly WebGLRenderer not having side implemented?

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Oh wait, yes, something broke at some point. Looking into it...

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Ah. It's the colors... my fault. Fixing.

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 6, 2012

Don't do anything, I already committed fix, more things to come.

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 6, 2012

Ah, ok cool!

@alteredq

This comment has been minimized.

Contributor

alteredq replied Aug 7, 2012

Ok, all examples should work now. BTW there were quite a few broken from since before these changes ;/

@mrdoob

This comment has been minimized.

Owner

mrdoob replied Aug 7, 2012

Sweet sweet!

Please sign in to comment.