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

MapblockMeshGenerator: Use more verbose member names #13244

Merged
merged 3 commits into from Oct 3, 2023

Conversation

Desour
Copy link
Member

@Desour Desour commented Feb 22, 2023

  • Goal of the PR: The code in content_mapblock.cpp is kinda hard to read due to heavy use of member variables that look like local stack variables (and quite non-specific variable names, like origin or color). All in all, it's hard to find out where something is defined or written to. Sometimes there are also name clashes (i.e. tile in getTile).
  • What this PR does:
    • Member vars were made private and const if possible.
    • Member vars are grouped into structs.
    • Member vars (or the structs they're in) start with m_.
  • This is a maintanance / code quality PR, without functional changes.

To do

This PR is a Ready for Review.

How to test

  • Skim through and make sure everything works the same as before.
    (The diff is large, but very simple.)

@sfan5
Copy link
Member

sfan5 commented Feb 23, 2023

@numberZero do you have an opinion on this?

@numberZero
Copy link
Contributor

Member vars were made private and const if possible.

👍

Member vars (or the structs they're in) start with m_.

👎 That adds more clutter than help IMO.

Member vars are grouped into structs.

For drawtype-specific stuff, that’s better than currently. But maybe it is possible to split the class instead?

@numberZero
Copy link
Contributor

Also, meshmanip and enable_mesh_cache are specific to mesh nodes

@Desour
Copy link
Member Author

Desour commented Feb 23, 2023

Also, meshmanip and enable_mesh_cache are specific to mesh nodes

They are not specific for the current mesh node though.
The structs are only valid if currently the specific drawtype is drawn.

Member vars (or the structs they're in) start with m_.

-1 That adds more clutter than help IMO.

I guess this also goes down to personal preference. I'll remove the m_ then if nobody else favours it.
Btw. how about cliquid for "current liquid" (and the same for plant, ...)?

Member vars are grouped into structs.

For drawtype-specific stuff, that’s better than currently. But maybe it is possible to split the class instead?

Splitting it would be good at some point I guess. But idk how to make it well, and rebasing would be harder.

@numberZero
Copy link
Contributor

They are not specific for the current mesh node though.
The structs are only valid if currently the specific drawtype is drawn.

Okay.

how about cliquid

cur_liquid would be better IMO.

But idk how to make it well

Neither I do, so nvm. That is for another PR.

@Desour
Copy link
Member Author

Desour commented Feb 24, 2023

Done.

@numberZero
Copy link
Contributor

LGTM

@Desour Desour force-pushed the mapblock_mesh_generator_refac branch 2 times, most recently from 30d95ba to 63a1ba6 Compare April 9, 2023 01:06
@Desour
Copy link
Member Author

Desour commented Apr 9, 2023

Rebased.

@rubenwardy rubenwardy added the Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements label May 27, 2023
@SmallJoker SmallJoker added the Rebase needed The PR needs to be rebased by its author. label Sep 23, 2023
@Desour Desour force-pushed the mapblock_mesh_generator_refac branch from 63a1ba6 to bbd9a01 Compare September 29, 2023 15:44
=> more consistency with neighbordata. and to emphasize that it's not the
same content_t, but the same liquid
@Desour Desour removed the Rebase needed The PR needs to be rebased by its author. label Sep 29, 2023
@Desour
Copy link
Member Author

Desour commented Sep 29, 2023

Rebased, and checked that the rebase went correctly.

Also added 2 commits, see commit descriptions.

@Desour Desour merged commit 8db4381 into minetest:master Oct 3, 2023
13 checks passed
@Desour Desour deleted the mapblock_mesh_generator_refac branch October 3, 2023 22:28
Zughy pushed a commit to Zughy/minetest that referenced this pull request Oct 8, 2023
kawogi pushed a commit to kawogi/minetest that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements One approval ✅ ◻️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants