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

Firelike drawtype: Simplify and improve #3171

Closed
wants to merge 1 commit into from
Closed

Firelike drawtype: Simplify and improve #3171

wants to merge 1 commit into from

Conversation

paramat
Copy link
Contributor

@paramat paramat commented Sep 12, 2015

Remove 2 central diagonal textures
Remove unusable param2 fine rotation that would break
orientation to neighbouring nodes
Replace top 4 textures with 1, spaced by 1/16 node
The new total of 5 fixes top textures missing in certain
circumstances
Tilt textures in by 22.5 (90/4) degrees instead of 10
Use floats, not ints, for vOffset and hOffset, and tune these
Fix code style issues and long lines

Current firelike has a problem, it uses 10 textures within a limit of 6. The lower 4 side flames override the top 4 flames (the burning underside of the node above). Depending on which of the lower side flames are used some of the top textures are missing. Also, using 4 flames for the single node surface above is over-complex.

With this commit:

screenshot_20150912_054644

^ On it's own has a pyramid campfire look.

screenshot_20150912_054613

^ Textures are tilted in more for more density when seen from above.

This commit removes the 2 central diagonal flames and uses a single top flame, for a new total of 5 textures, because this is less than the limit of 6 these are all independant so none will be missing in certain circumstances.

The top texture is spaced by a single 16px texture pixel from the node above:

screenshot_20150912_054356

The side flames are tilted in more (from 10 to 22.5 degrees) to help with visual flame density. I was considering what angles are suitable in Minetest, we have 90 and 45 degree angles which are 360 / 2^n, repeated division of 360 by 2, which results in 22.5 degrees.

I removed the fine-step rotation by param2 (a feature recently added to plantlike by RealBadAngel), this would be unusable because the flames of firelike need to be orientated to surrounding nodes.

'int's were being used for vOffset and hOffset, i changed these to 'float's for the necessary accuracy in positioning the textures to meet the lower edges.

Firelike code is now more lightweight during mesh generation, the fewer textures will help with the significant FPS drop of a large fire which players have reported.

@paramat paramat added enhancement @ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements labels Sep 12, 2015
@est31
Copy link
Contributor

est31 commented Sep 12, 2015

Firelike code is now more than twice as lightweight

This is only mesh making, so shouldn't help with the FPS problem at all.

@est31
Copy link
Contributor

est31 commented Sep 12, 2015

Admittedly, there isn't much of a difference if you look at the fire from the sides, but you shouldn't look at it from above anymore:

Before:
screenshot_20150912_033053
After:
screenshot_20150912_033128

@est31
Copy link
Contributor

est31 commented Sep 12, 2015

I don't know, can irrlicht be ordered to only render certain faces when looking at them from certain angles? could the two center faces are added again and only rendered when you look at them from higher degrees than e.g. 45°?

@paramat
Copy link
Contributor Author

paramat commented Sep 12, 2015

Hm, it is a bit ugly, maybe we could tilt the 4 flames in more.
I find the current box with a cross fairly ugly too.

only rendered when you look at them from higher degrees

I think that would be visually annoying, it's a messy and overcomplex method too.

@est31
Copy link
Contributor

est31 commented Sep 12, 2015

👎
What Param2 means is chosen by the subgame inside the nodedef not by the engine. Not all nodes have to be param2, and if neighbouring nodes are influenced by firelike's param2, then that's a bug in the rotation code. It must only respect param2 if the neighbouring node's nodedef also has an according paramtype2=facedir.

Param2 is not and should not be reserved for rotation. If a subgame does that in order to have higher speed (which is a quite small win compared that you sacrifice the whole param2 field for facedir only), fine then they should only use it for that.

The only reason why removing param2 here might be acceptable, is that it isn't used by mods, but there is totally no reason why it is "unusable" as you claim.

@paramat
Copy link
Contributor Author

paramat commented Sep 12, 2015

This is only mesh making, so shouldn't help with the FPS problem at all.

Yes i agree. But it will help at mesh creation.

To be clear, the rotation br param2 i removed here is the fine-step rotation added to plantlike by RBA, not facedir. It added complexity to the code which would perhaps never be usable because firelike needs to be orientated to the grid.

I might try tilting the 4 textures in more, by 22.5 degrees.

@est31
Copy link
Contributor

est31 commented Sep 12, 2015

To be clear, the rotation br param2 i removed here is the fine-step rotation added to plantlike by RBA, not facedir. It added complexity to the code which would perhaps never be usable because firelike needs to be orientated to the grid.

You claimed that it is not useable because it causes wrong facedir orientations for neighbouring nodes, no?

Remove 2 central diagonal textures
Remove unusable param2 fine rotation that would break
orientation to neighbouring nodes
Replace top 4 textures with 1, spaced by 1/16 node
The new total of 5 fixes top textures missing in certain
circumstances
Tilt textures in by 22.5 (90/4) degrees instead of 10
Use floats, not ints, for vOffset and hOffset, and tune these
Fix code style issues and long lines
@paramat
Copy link
Contributor Author

paramat commented Sep 12, 2015

I meant i removed the ' + n.param2 * 2' from for example:
vertices[i].Pos.rotateXZBy(90 + n.param2 * 2);
This has nothing to do with facedir, this is param2 being used for a very fine rotation control, just as in the plantlike rotation feature by RBA. The authors of firelike drawtype added it for some reason without realising it would break the side flames lining up with neighbouring nodes.

Anyway, i agree with you about my commit looking ugly from above, so have tilted the textures in more, first post updated.

@paramat
Copy link
Contributor Author

paramat commented Sep 12, 2015

Heh after all that i've changed my mind. Tilting the side flames in more may improve density, but these are then too far away from neighbour nodes. Complex as it is the 6 lower flames seem to be needed. I also realsed the top 4 flames work okay as they are due to the logic.

I'll open a new PR with just the code cleanup and simplifications, removal of fine-rotation, and slightly adjust the angles used to 11.25 and 22.5 degrees.

@paramat paramat closed this Sep 12, 2015
@paramat paramat deleted the firelike branch September 14, 2015 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants