realXtend / tundra

realXtend Tundra SDK, a 3D virtual world application platform.
www.realxtend.org
Apache License 2.0
84 stars 70 forks source link

Adminotech Tundra 2.4.1 fixes #600

Closed jonnenauha closed 11 years ago

jonnenauha commented 11 years ago
juj commented 11 years ago

This looks good. I am unfortunately not available to test this in January, so if you are comfortable with this, and have given some testing on Windows and OSX that you are satisfied, feel free to merge this in.

jonnenauha commented 11 years ago

@juj @cadaver @Stinkfist0 @cvetan5 We had this code in our 2.4.1 release and its worked ok. I still think we should revert the change of SetMaterial behavior or we change EC_Highlight to change the material without the local/disconnected attribute change. Many users are having problems with normal situations like: Copy and paste and entity that is open in a editor. Resulting in a entity that has the temporary EC_Highlight material in the attributes, and there is nothing to restore it to the original. Additionally I just think its super weird that for couple of years the behaviour has been to keep the original materials nicely in the EC editor and now we change it so that weird generated material names pop up in the editor and sometimes not (when you dont have manip widget shown). This is very confusing to people, even to me and I know whats going on.

TL;DR I suggest we change EC_Highlight to set the materials only to ogre and not the attribute list in EC_Mesh as its confusing and breaks things. We could also have SetOgreMaterial as a separate function and document it to say it does not edit the attribute but changes the material to ogre. Scripts/c++ could then choose the desired function.

Stinkfist0 commented 11 years ago

Looking good.

jonnenauha commented 11 years ago

@Stinkfist0 You mean I could change the EC_Highlight not to modify the attribute, or its looking good in general? :) I'd really like to have highlight not modify the material attribute list.

Stinkfist0 commented 11 years ago

Looking good in general. Regarding EC_Highlight: if it changes material only internally, the changes will not be replicated. This is probably not an issue though as the component can be considered being sort of local editor aid.

jonnenauha commented 11 years ago

No it wont be replicated, that would be disaster. Even if its local only change the attribute list in EC_Mesh in the editor is changed. Now if you copy paste the entity or something like that locally you have dead non working material refs in the attribute. The source objects highlight wont restore the original material in anything but its target EC_Mesh.

Yes I feel it should indeed be a local editor. Now its manipulating the scene state (even if its local only) and its not a good place to be in. Tweaking scene state when ever the manipulator widget is toggled is bad, especially when its visible to the end user via the editors and the benefit of showing a generated temp material name in the editor while its manipulated is absolutely zero.

Stinkfist0 commented 11 years ago

Maybe we should merge this soon? The bugginess of the highlighting is really annoying at the moment.

jonnenauha commented 11 years ago

Yep it was :) You are welcome to hit the merge button any time once you've reviewed from Ludos side it looks ok. In my opinion the code is good and has been in for the most part in our 2.4.1 release and the last commits went in today to our 2.4.1.1 release.

I guess the only question may be if @juj wants EC_Mesh and EC_Highlight to behave this way. I only see problems in what they were doing before these fixes, no real upsides. Maybe transparency so the attributes in EC editor wont "lie" but I think the problems what you get from it for the end users trump that. This transparency also caused several bugs in different kinds of load/failure situations.

Stinkfist0 commented 11 years ago

I'll test the code in practice probably on Monday and merge it after that.

antont commented 11 years ago

If we merge this now as is, but later want to change the attribute change handling / how setmaterial with highlight etc. to fix the prob some other way, does it go cleanly enough or become somehow messy due to this fix already being in?

I think it is not much code and possible changes can be done later just by editing the code, or possibly by reverting the commits related to that or something.

So I figure is good to merge.

Stinkfist0 commented 11 years ago

Yup. I think anyone can merge this in as long as he makes sure to test that this works ok. As said, I can do this probably on next Monday, but if somebody can do this before that, please freel free to do so.

Stinkfist0 commented 11 years ago

Don't have the time for this today. @cadaver however might take a look.

cadaver commented 11 years ago

Seems to be working OK. I'm merging now.