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

Adding particle blend, glow and animation #4774

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@sfan5
Member

sfan5 commented Nov 14, 2016

see #4705

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 14, 2016

Member

Comment copied over:

I have a request that may be acceptable to the author, something essential.
The blend API should be focussed on the majority of users who use the basic example values, not the minority using the technical blend parameters.
Currently casual modders will be using:
material_type_param = 12641,
Which is not acceptable API, the parameter name is technical and unclear, and the value used an abstract number.
I request that the parameter is called 'blend_type =' or similar, and that it also accepts values for the basic essential blend types that are clear English: "additive", "subtractive', "inverted".
This way the technical blend options are completely hidden for the majority of users.

Also please see my line comments in the original. Any new line comments i will make here.

Member

paramat commented Nov 14, 2016

Comment copied over:

I have a request that may be acceptable to the author, something essential.
The blend API should be focussed on the majority of users who use the basic example values, not the minority using the technical blend parameters.
Currently casual modders will be using:
material_type_param = 12641,
Which is not acceptable API, the parameter name is technical and unclear, and the value used an abstract number.
I request that the parameter is called 'blend_type =' or similar, and that it also accepts values for the basic essential blend types that are clear English: "additive", "subtractive', "inverted".
This way the technical blend options are completely hidden for the majority of users.

Also please see my line comments in the original. Any new line comments i will make here.

@Foghrye4

This comment has been minimized.

Show comment
Hide comment
@Foghrye4

Foghrye4 Nov 14, 2016

Contributor

@sfan5 should I interfere or you will do all job for me?

Contributor

Foghrye4 commented Nov 14, 2016

@sfan5 should I interfere or you will do all job for me?

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Nov 14, 2016

Member

@paramat

I request that the parameter is called 'blend_type =' or similar, and that it also accepts values for the basic essential blend types that are clear English: "additive", "subtractive', "inverted".

How about adding a table minetest.blend_types which contains these predefined values? That would allow all the configurability without needing to add seperate handling for "normal" blend types.

Also please see my line comments in the original. Any new line comments i will make here.

Please re-comment on this pull so others (and I) can clearly see which comments still apply and which don't.

Member

sfan5 commented Nov 14, 2016

@paramat

I request that the parameter is called 'blend_type =' or similar, and that it also accepts values for the basic essential blend types that are clear English: "additive", "subtractive', "inverted".

How about adding a table minetest.blend_types which contains these predefined values? That would allow all the configurability without needing to add seperate handling for "normal" blend types.

Also please see my line comments in the original. Any new line comments i will make here.

Please re-comment on this pull so others (and I) can clearly see which comments still apply and which don't.

Show outdated Hide outdated doc/lua_api.txt
-- specified material type param, refer to "Particle blend" section
animation = {Animation definition},
-- ^ optional, note that particle and particle spawners have differences
glow = 0,

This comment has been minimized.

@paramat

paramat Nov 14, 2016

Member

Apparently 'Particle brightness = min(environment light level + glow, 15).'
I remember @sofar was asking for a fixed value, maybe how this behaves needs some consideration. For example fire particles would tend to be the same brightness independent of ambient light.
I think i might prefer a fixed value instead of adding to ambient light.

@paramat

paramat Nov 14, 2016

Member

Apparently 'Particle brightness = min(environment light level + glow, 15).'
I remember @sofar was asking for a fixed value, maybe how this behaves needs some consideration. For example fire particles would tend to be the same brightness independent of ambient light.
I think i might prefer a fixed value instead of adding to ambient light.

This comment has been minimized.

@sfan5

sfan5 Nov 14, 2016

Member

IMO both ways have lots of potential for use by modders

@sfan5

sfan5 Nov 14, 2016

Member

IMO both ways have lots of potential for use by modders

This comment has been minimized.

@Foghrye4

Foghrye4 Nov 15, 2016

Contributor

@paramat @sfan5 Obviously fire should have glow level 15. If we use fixed value 4-8 independed from environment light level and emit this particle in full bright area, this particle will be visually darker, than same particle in dark area. Which is quite strange behaviour for material.

So, paramat, should we consider your comment as a feature request?

@Foghrye4

Foghrye4 Nov 15, 2016

Contributor

@paramat @sfan5 Obviously fire should have glow level 15. If we use fixed value 4-8 independed from environment light level and emit this particle in full bright area, this particle will be visually darker, than same particle in dark area. Which is quite strange behaviour for material.

So, paramat, should we consider your comment as a feature request?

This comment has been minimized.

@paramat

paramat Nov 15, 2016

Member

Indeed. An alternative i suggested a while ago is
particle light = max(glow_value, ambient_light)
I have no idea what would be best, still considering.

@paramat

paramat Nov 15, 2016

Member

Indeed. An alternative i suggested a while ago is
particle light = max(glow_value, ambient_light)
I have no idea what would be best, still considering.

This comment has been minimized.

@Foghrye4

Foghrye4 Nov 15, 2016

Contributor

@paramat also consider how we should check what type of glow to use. Should it be boolean or string enum. And what name of a parameter to use and what options in enum (if use it).

@Foghrye4

Foghrye4 Nov 15, 2016

Contributor

@paramat also consider how we should check what type of glow to use. Should it be boolean or string enum. And what name of a parameter to use and what options in enum (if use it).

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 14, 2016

Member

Transferred line comments.

I assume you mean a table containing {"additive" = 12641. "subtractive" = 12548, ..} so that the English words can be used, but also more entries could be added easily later?

Member

paramat commented Nov 14, 2016

Transferred line comments.

I assume you mean a table containing {"additive" = 12641. "subtractive" = 12548, ..} so that the English words can be used, but also more entries could be added easily later?

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 14, 2016

Member

@Foghrye4 please do interfere, you are the original author after all.

Member

paramat commented Nov 14, 2016

@Foghrye4 please do interfere, you are the original author after all.

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Nov 14, 2016

Member

I assume you mean a table containing {"additive" = 12641. "subtractive" = 12548, ..} so that the English words can be used, but also more entries could be added easily later?

Yes

Member

sfan5 commented Nov 14, 2016

I assume you mean a table containing {"additive" = 12641. "subtractive" = 12548, ..} so that the English words can be used, but also more entries could be added easily later?

Yes

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 15, 2016

Member

Seems good to me.

Member

paramat commented Nov 15, 2016

Seems good to me.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 15, 2016

Member

The main issue seems to be the documentation for the blend options. I've tried reading them a few times and my head just starts to hurt. Celeron55 found them a little confusing too, see his comments on the PR #4705 (comment)

I'm not too bothered about the 2D frame sheets anymore, not really necessary but i'm fairly neutral now.

Member

paramat commented Nov 15, 2016

The main issue seems to be the documentation for the blend options. I've tried reading them a few times and my head just starts to hurt. Celeron55 found them a little confusing too, see his comments on the PR #4705 (comment)

I'm not too bothered about the 2D frame sheets anymore, not really necessary but i'm fairly neutral now.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 15, 2016

Member

Is 'inverted' or 'inverse' a better word than 'invert' for the blend type? These are better English than 'invert blend'. I think perhaps 'inverted' is best, that word seems used a lot on opengl sites.

Member

paramat commented Nov 15, 2016

Is 'inverted' or 'inverse' a better word than 'invert' for the blend type? These are better English than 'invert blend'. I think perhaps 'inverted' is best, that word seems used a lot on opengl sites.

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Nov 15, 2016

Member

Is 'inverted' or 'inverse' a better word than 'invert' for the blend type? These are better English than 'invert blend'.

In my experience with colors you usually use "invert" not "inverse". Renaming it to inverted sounds okay however.

Member

sfan5 commented Nov 15, 2016

Is 'inverted' or 'inverse' a better word than 'invert' for the blend type? These are better English than 'invert blend'.

In my experience with colors you usually use "invert" not "inverse". Renaming it to inverted sounds okay however.

@raymoo raymoo referenced this pull request Nov 20, 2016

Closed

Lit Particle Effects. #4215

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Nov 20, 2016

Contributor

@sfan5 when merging can you please squash all of your commits into one? So that there is one commit by @Foghrye4 and one by you.

Contributor

est31 commented Nov 20, 2016

@sfan5 when merging can you please squash all of your commits into one? So that there is one commit by @Foghrye4 and one by you.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 20, 2016

Member

@sfan5 yes there's still improvement needed, see celeron55's comment #4705 (comment) and i agree, the documentation for blend types needs a rewrite to be much clearer and assume less technical knowledge.

Also, hmmmm has started reviewing but not finished.

To be clear, i support the basic intent of this PR. I want particle animation and glow, i almost started coding glow myself. While i don't see much need for blend options i don't mind a few basic ones being available. I still disagree with having the technical blend options, but seeing the support for this i have little hope of stopping those.

Member

paramat commented Nov 20, 2016

@sfan5 yes there's still improvement needed, see celeron55's comment #4705 (comment) and i agree, the documentation for blend types needs a rewrite to be much clearer and assume less technical knowledge.

Also, hmmmm has started reviewing but not finished.

To be clear, i support the basic intent of this PR. I want particle animation and glow, i almost started coding glow myself. While i don't see much need for blend options i don't mind a few basic ones being available. I still disagree with having the technical blend options, but seeing the support for this i have little hope of stopping those.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 20, 2016

Member

The parameter material_type_param = 0, should i feel be renamed to be clear, less technical and focussed on the non-technical modder who will use the simple examples stored in the minetest.blend_types table, 'material type' does not refer to blend for the majority of modders. How about 'blend_type'?
Will we be able to use the parameter like this:
blend_type = additive
or is it
= minetest.blend_types.additive
?

Member

paramat commented Nov 20, 2016

The parameter material_type_param = 0, should i feel be renamed to be clear, less technical and focussed on the non-technical modder who will use the simple examples stored in the minetest.blend_types table, 'material type' does not refer to blend for the majority of modders. How about 'blend_type'?
Will we be able to use the parameter like this:
blend_type = additive
or is it
= minetest.blend_types.additive
?

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Nov 20, 2016

Member

http://irc.minetest.net/minetest-dev/2016-11-14#i_4742209
11:48 celeron55 regardless, i do think the documentation is bad
11:50 the api or system doesn't use any concepts that i don't understand yet it's still difficult for me to understand what's the best way to use the api

Member

paramat commented Nov 20, 2016

http://irc.minetest.net/minetest-dev/2016-11-14#i_4742209
11:48 celeron55 regardless, i do think the documentation is bad
11:50 the api or system doesn't use any concepts that i don't understand yet it's still difficult for me to understand what's the best way to use the api

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 1, 2016

Member

If even celeron55 doesn't understand it it's not an acceptable API.

Member

paramat commented Dec 1, 2016

If even celeron55 doesn't understand it it's not an acceptable API.

@asl97

This comment has been minimized.

Show comment
Hide comment
@asl97

asl97 Dec 1, 2016

Contributor

@paramat There need to be a limit to how much should a feature require simplifying.
We do not want to over simplify it till it become useless for both the pull request creator and others.

Blocking a feature just because one core dev doesn't understand it isn't a good reason, even if that core dev is the creator.

Everyone is good at something and sometimes you just have to let them take charge of the parts they are good at.

Contributor

asl97 commented Dec 1, 2016

@paramat There need to be a limit to how much should a feature require simplifying.
We do not want to over simplify it till it become useless for both the pull request creator and others.

Blocking a feature just because one core dev doesn't understand it isn't a good reason, even if that core dev is the creator.

Everyone is good at something and sometimes you just have to let them take charge of the parts they are good at.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 1, 2016

Member

My requests are nowhere near for 'over simplifying' they are more a request for a little less extreme complexity. Having blend options for particles is a fairly powerful and specialised feature already, and i support having the choice of a few basic blend types.

Member

paramat commented Dec 1, 2016

My requests are nowhere near for 'over simplifying' they are more a request for a little less extreme complexity. Having blend options for particles is a fairly powerful and specialised feature already, and i support having the choice of a few basic blend types.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 1, 2016

Member

The original author has dissociated themselves from this PR, so we are free to do what we want, and here is my suggestion:

Everyone, including me, wants particle light and animation, and i really don't mind having a few basic blend types selectable. However having complete fine control over the combination and multiplication of channel values is overkill, few people understand it or need it and few will use it.

So i suggest that the blend fine control is removed, and we have a parameter 'blend_type' that can be set to "additive" "subtractive" "inverted" and maybe 1 or 2 more. The channel effects of these is hardcoded and we can always add additional basic blend types in future.

Member

paramat commented Dec 1, 2016

The original author has dissociated themselves from this PR, so we are free to do what we want, and here is my suggestion:

Everyone, including me, wants particle light and animation, and i really don't mind having a few basic blend types selectable. However having complete fine control over the combination and multiplication of channel values is overkill, few people understand it or need it and few will use it.

So i suggest that the blend fine control is removed, and we have a parameter 'blend_type' that can be set to "additive" "subtractive" "inverted" and maybe 1 or 2 more. The channel effects of these is hardcoded and we can always add additional basic blend types in future.

@asl97

This comment has been minimized.

Show comment
Hide comment
@asl97

asl97 Dec 2, 2016

Contributor

They have also dissociated themselves from this PR

I would probably have done the same, (imo) there are a number of bad choices like forcing people to use the key table name.

However having complete fine control over the combination and multiplication of channel values is overkill, few people understand it or need it and few will use it.

Couldn't the same thing be said of the noise generator?

So i suggest that the blend fine control is removed, and we have a parameter 'blend_type' that can be set to "additive" "subtractive" "inverted" and maybe 1 or 2 more.

What's wrong with having fine control being there? It's not like people need to use them.
There is already the horrible table system.

It's like suggesting to replace the Noise Parameters system with "flat", "default", "hilly" and maybe 1 or 2 more

Contributor

asl97 commented Dec 2, 2016

They have also dissociated themselves from this PR

I would probably have done the same, (imo) there are a number of bad choices like forcing people to use the key table name.

However having complete fine control over the combination and multiplication of channel values is overkill, few people understand it or need it and few will use it.

Couldn't the same thing be said of the noise generator?

So i suggest that the blend fine control is removed, and we have a parameter 'blend_type' that can be set to "additive" "subtractive" "inverted" and maybe 1 or 2 more.

What's wrong with having fine control being there? It's not like people need to use them.
There is already the horrible table system.

It's like suggesting to replace the Noise Parameters system with "flat", "default", "hilly" and maybe 1 or 2 more

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Dec 2, 2016

Member

@paramat

And the documentation overall is unclear, see celeron55's comments in the original PR

what celeron55 said relates to the old documentation before I improved it (or at least tried)

Could we have explanation of each simple blend type, plus an example?

I can add those, but what do you mean by "an example"? possible usage?

Instead could we have a simpler but still powerful set of parameters for blend types that are less technical?

If you don't like how it is please suggest something better. By the way having four predefined parameters is not simpler but powerful

Member

sfan5 commented Dec 2, 2016

@paramat

And the documentation overall is unclear, see celeron55's comments in the original PR

what celeron55 said relates to the old documentation before I improved it (or at least tried)

Could we have explanation of each simple blend type, plus an example?

I can add those, but what do you mean by "an example"? possible usage?

Instead could we have a simpler but still powerful set of parameters for blend types that are less technical?

If you don't like how it is please suggest something better. By the way having four predefined parameters is not simpler but powerful

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 2, 2016

Member

I can add those, but what do you mean by "an example"? possible usage?

Yes, just a simple example like 'fire' or 'smoke', I feel the effect of the blend type should be described, as most don't know what 'additive' 'subtractive' 'inverted' means (i don't).
You have improved the documentation slightly but it's basically the same.

Instead could we have a simpler but still powerful set of parameters for blend types that are less technical?

I'm not suggesting this now.

If you don't like how it is please suggest something better

My current suggestion is #4774 (comment)

Member

paramat commented Dec 2, 2016

I can add those, but what do you mean by "an example"? possible usage?

Yes, just a simple example like 'fire' or 'smoke', I feel the effect of the blend type should be described, as most don't know what 'additive' 'subtractive' 'inverted' means (i don't).
You have improved the documentation slightly but it's basically the same.

Instead could we have a simpler but still powerful set of parameters for blend types that are less technical?

I'm not suggesting this now.

If you don't like how it is please suggest something better

My current suggestion is #4774 (comment)

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Dec 2, 2016

Member

You have improved the documentation slightly but it's basically the same.

I have pretty much rewritten the documentation entirely while keeping what it says. Failure to understand technical terms used does not equate bad documentation.

My current suggestion is #4774 (comment)

Your suggestion is to throw all this PR is about into the trash because "modders are just too stupid to use this"... Let me just mention the analogy with noise params here.

Member

sfan5 commented Dec 2, 2016

You have improved the documentation slightly but it's basically the same.

I have pretty much rewritten the documentation entirely while keeping what it says. Failure to understand technical terms used does not equate bad documentation.

My current suggestion is #4774 (comment)

Your suggestion is to throw all this PR is about into the trash because "modders are just too stupid to use this"... Let me just mention the analogy with noise params here.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 2, 2016

There is no need to remove the "blend fine control". Why? Let the modders that don't understand blend use the pre-defined blend options, and let those that do know use whatever they want. It's perfectly acceptable. Every software has its normal and its power users. I fail to see why it so much of a concern that there is total flexibility in blend.

ghost commented Dec 2, 2016

There is no need to remove the "blend fine control". Why? Let the modders that don't understand blend use the pre-defined blend options, and let those that do know use whatever they want. It's perfectly acceptable. Every software has its normal and its power users. I fail to see why it so much of a concern that there is total flexibility in blend.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 2, 2016

Member

Yes i agree the documentation cannot really be simplified because it is for complex parameters, and clarification is very difficult, so it's best i stay neutral on that now.

I suspected someone would mention mapgen stuff as an analogy, however it's a poor argument, fine control of perlin noise is actually needed, fundamental and essential.

I'll say again that i'm not blocking anything, i can't block this, this has enough support to merge even if i object, the other devs have been very kind in considering my objections, which is what we tend to do when there is controversy.

Member

paramat commented Dec 2, 2016

Yes i agree the documentation cannot really be simplified because it is for complex parameters, and clarification is very difficult, so it's best i stay neutral on that now.

I suspected someone would mention mapgen stuff as an analogy, however it's a poor argument, fine control of perlin noise is actually needed, fundamental and essential.

I'll say again that i'm not blocking anything, i can't block this, this has enough support to merge even if i object, the other devs have been very kind in considering my objections, which is what we tend to do when there is controversy.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 2, 2016

Member

a number of bad choices like forcing people to use the key table name.

It's a little messy, but the original use of abstract numbers was very bad API design, unnacceptable.

Member

paramat commented Dec 2, 2016

a number of bad choices like forcing people to use the key table name.

It's a little messy, but the original use of abstract numbers was very bad API design, unnacceptable.

@asl97

This comment has been minimized.

Show comment
Hide comment
@asl97

asl97 Dec 2, 2016

Contributor

@paramat Being able to use an named variable or a named key in a table is different from being force to use a key in the table.

Passing a string is just ugly messy, the formal optional table access was fine.

An example of a widely use example is the re module from python, it has a bunch of flag like re.DOTALL but it doesn't force user to use it or "DOTALL", they can if they want to use 16 directly too.

For example, we can't pass 1 or blend_ebf.one to pack_texture_blend_func, we HAVE to pass "one".

Contributor

asl97 commented Dec 2, 2016

@paramat Being able to use an named variable or a named key in a table is different from being force to use a key in the table.

Passing a string is just ugly messy, the formal optional table access was fine.

An example of a widely use example is the re module from python, it has a bunch of flag like re.DOTALL but it doesn't force user to use it or "DOTALL", they can if they want to use 16 directly too.

For example, we can't pass 1 or blend_ebf.one to pack_texture_blend_func, we HAVE to pass "one".

@kaeza

This comment has been minimized.

Show comment
Hide comment
@kaeza

kaeza Dec 3, 2016

Contributor

@asl97 The issue is consistency. In Python, modules usually export things as integer "constants", but in Lua it is usually done by mapping strings to numbers. It should be possible to pass numbers if that's desired, though.

Contributor

kaeza commented Dec 3, 2016

@asl97 The issue is consistency. In Python, modules usually export things as integer "constants", but in Lua it is usually done by mapping strings to numbers. It should be possible to pass numbers if that's desired, though.

@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Dec 3, 2016

Contributor

Hang on. How is this controversial? At the time the label was added, only 2 people had commented. That's not controversial... I'm confused.

Contributor

Zeno- commented Dec 3, 2016

Hang on. How is this controversial? At the time the label was added, only 2 people had commented. That's not controversial... I'm confused.

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Dec 3, 2016

Member

@Zeno- I've added the label because of the controversy with the old PR.

@kaeza the currect code is specially designed NOT to expose the modders to the irrlicht constants, e.g. their values are not stated in the docs.

Member

sfan5 commented Dec 3, 2016

@Zeno- I've added the label because of the controversy with the old PR.

@kaeza the currect code is specially designed NOT to expose the modders to the irrlicht constants, e.g. their values are not stated in the docs.

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Dec 3, 2016

Member

My only remaining suggestions are:
Allow blend_type to accept the strings "additive" etc. however i understand that this requires more complex handling, so is just a suggestion.
In the documentation, short non-technical explanations of what "subtractive" and "inverted" blend do, "additive" already has a nice example that shows that translucent areas brighten with overlap.

Thanks to sfan5 for considering my suggestions, and sorry to all for making such a fuss and occasionally being unpleasant.

Member

paramat commented Dec 3, 2016

My only remaining suggestions are:
Allow blend_type to accept the strings "additive" etc. however i understand that this requires more complex handling, so is just a suggestion.
In the documentation, short non-technical explanations of what "subtractive" and "inverted" blend do, "additive" already has a nice example that shows that translucent areas brighten with overlap.

Thanks to sfan5 for considering my suggestions, and sorry to all for making such a fuss and occasionally being unpleasant.

@sfan5 sfan5 removed this from the 0.4.15 milestone Dec 7, 2016

@sofar

This comment has been minimized.

Show comment
Hide comment
@sofar

sofar Dec 13, 2016

Member

git am 4774.patch -> trailing whitespace, can't apply?

Member

sofar commented Dec 13, 2016

git am 4774.patch -> trailing whitespace, can't apply?

@Ekdohibs

This comment has been minimized.

Show comment
Hide comment
@Ekdohibs

Ekdohibs Jan 12, 2017

Member

Removed 'controversial' label since @paramat seems to agree.
@sfan5: could you please rebase this?

Member

Ekdohibs commented Jan 12, 2017

Removed 'controversial' label since @paramat seems to agree.
@sfan5: could you please rebase this?

@sfan5

This comment has been minimized.

Show comment
Hide comment
@sfan5

sfan5 Jan 12, 2017

Member

i might rebase it sooner or later, it should be rewritten based on the new TileAnimation stuct anyway since the code shouldn't be duplicated

Member

sfan5 commented Jan 12, 2017

i might rebase it sooner or later, it should be rewritten based on the new TileAnimation stuct anyway since the code shouldn't be duplicated

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jan 12, 2017

Member

I still oppose this, specifically the complexity of blend options, but i can see that i can't stop it, so am making requests for improvements for if it goes ahead.

Member

paramat commented Jan 12, 2017

I still oppose this, specifically the complexity of blend options, but i can see that i can't stop it, so am making requests for improvements for if it goes ahead.

@Zeno-

This comment has been minimized.

Show comment
Hide comment
@Zeno-

Zeno- Jan 12, 2017

Contributor

I don't think this is controversial. Removed label

Contributor

Zeno- commented Jan 12, 2017

I don't think this is controversial. Removed label

@paramat

This comment has been minimized.

Show comment
Hide comment
@paramat

paramat Jan 12, 2017

Member

I'm not too bothered about having that label present, but the degree of argument here means it is well justified. However if someone is just going to remove it again i won't bother re-adding it.

Member

paramat commented Jan 12, 2017

I'm not too bothered about having that label present, but the degree of argument here means it is well justified. However if someone is just going to remove it again i won't bother re-adding it.

@sfan5 sfan5 closed this Jan 18, 2017

@sfan5 sfan5 deleted the sfan5:pull4705 branch Jan 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment