Door Fixes & Improvements #3479
No reviewers
ancientmarinerdev
Labels
No Label
#P1 CRITICAL
#P2: HIGH
#P3: elevated
#P4 priority: medium
#P6: low
#Review
annoying
API
bug
code quality
combat
commands
compatibility
configurability
contribution inside
controls
core feature
creative mode
delayed for engine release
documentation
duplicate
enhancement
environment
feature request
gameplay
graphics
ground content conflict
GUI/HUD
help wanted
incomplete feature
invalid / won't fix
items
looking for contributor
mapgen
meta
mineclone2+
Minecraft >= 1.13
Minecraft >= 1.17
missing feature
mobile
mobs
mod support
model needed
multiplayer
Needs adoption
needs discussion
needs engine change
needs more information
needs research
nodes
non-mob entities
performance
player
possible close
redstone
release notes
schematics
Skyblock
sounds
Testing / Retest
tools
translation
unconfirmed
mcl5
mcla
Media missing
No Milestone
No project
No Assignees
5 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: VoxeLibre/VoxeLibre#3479
Loading…
Reference in New Issue
No description provided.
Delete Branch "door_texture_fixes"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
This pull request fixes the issue where people had to mirror their door textures because the game used a different method to texture the doors.
Speaking of mirrored, this pull request also fixes mirrored doors and improves those greatly.
Door Texturing Fixesto WIP: Door Texturing FixesWIP: Door Texturing Fixesto Door Texturing FixesThanks for the refinements here! I'll test the mechanics tomorrow. For now, because PixelPerfection gave us headaches before, the first thing I looked at was the textures, to make sure they're not copied or modified versions of those in Minecraft.
Bad:
Oak: Minecraft derivative - can't accept it
Bamboo: Minecraft derivative - can't accept it
Problematic:
Iron: not copied, but it's black instead of silver
Acacia: ok, but the colors would need adjustment to fit the rest of the wood
Dark Oak: ok, the colors would need adjustment to fit the rest of the wood
Warped: the new door doesn't (but should) match our Warped Hyphae color, which is purple
Good:
Crimson: identical with ours, but slightly larger in file size
Birch: identical with ours, each (top/bottom) larger by 1 byte :P
Jungle: identical to ours, this time smaller in file size (Yay!)
Mangrove: identical, also smaller in file size (2xYay!)
Spruce: identical, smaller in file size (3xYay!)
This has now been changed.
This has now been changed.
This has now been changed.
This has now been changed.
The colour difference is very negatable. I decided not to change this unless absolutely necessary.
This has now been changed. The previous textures have been added as _alt versions.
About the oak and bamboo doors, how is having the same shape now considered a "derivative work"? The texturing is vastly different, so this shouldn't matter, right?
And as for the warped colours, in my personal opinion, they should be teal instead of purple. No need to try and be "cool" and "unique" for no reason.
It isn't about being cool. Our biome and most textures are purple. We need to be consistent. Changing 1 texture only isn't a good call and it is hard to justify just because there is a desire to be identical to MC.
Well, I am always here if you want me to convert every purple warped colour into the teal ones.
That one was personal to me, but yeah, I shouldn't have gotten carried away there.
I appreciate the offer. I'm squarely in camp purple though.
I removed it after, it wasn't the worst comment in the world but annoying based on previous comments. Nicu is trying to help. He didn't have to review during this time but I am happy he is. He's also quite partial to MC styles so you aren't winning potential allies in the debate!
My views are based on a good consistent game with unique things about it. I wasn't here when the purple happened but I'll happily defend it now it's a fact on the ground. I think it's safe to say I don't do anything because its cool. Unique maybe, but you know my reasoning. We've discussed look and feel before and I'm sure we will in the future :)
I appreciate your continued work and motivation as always.
Thanks for the changes!
You're right, not a big difference, but we'll get more wood types in the future and we need each to have a homogenous color palette, so they can easily be identified. It's worth doing it now if we're doing changes anyway.
Thanks, the new one is too dark and the hinges are barely visible against the dark wood. Can you brighten it up to match the planks? And now that I compared them, I see the top of warped trapdoor is too saturated. If you can tone that down to plank level, that would be nice.
For the bamboo door, I'd use the one introduced in 0.82. When I made it, I avoided the Minecraft-style cutout both because it's a good idea to differentiate, but also because I wanted it to give it a bit of character. That's why the bamboo stalks are cut between their nodes, with an offset, so its not a perfect square and for a more natural look. And it looks better than their door, like the acacia door that you added. Our doors are less boring than Minecraft's, and I like that.
Derivatives are generally not a good idea, it just makes it super easy for lawyers to take down stuff. It's more than enough that we have so much of their gameplay and mechanics, we could really use less similarity.
I wanted it teal too, but considering that this fungi/tree is Minecraft-specific, it's probably a better idea to have it purple.
About the side textures that got removed, it works nicely for most doors, but:
This is a weird argument. Every wood type that Minecraft has added, has had a unique colour to easily differentiate it from the other wood types. No wood type is going to get added soon which has a very similar colour to dark oak.
This has now been fixed.
When was this comment made? I designed my own bamboo door already from the PixelPerfection bamboo planks.
Well, I have good reason to believe that Minecraft will add purple wood and planks in the future, so what will we do when that time comes? If my assumptions are correct, then the question isn't if we are going to make the warped colour teal, but when.
Minecraft could very well do an ender update in the future and add purple wood that way.
Well, the way Minecraft does it, is that they use meshes for the door parts, which allows them to texture the doors much better and more easily. (They also use meshes for trapdoors as well!)
Sadly, all of these complaints are because of the limitations of nodeboxes. If you want these issues resolved without adding a bunch of texture bloat, then meshes are the way to go.
Besides, meshes could be a great idea, since all one needs to do is define a single top texture for the top mesh and one single bottom texture for the bottom mesh, and the texturing automatically happens correctly without having to mess around with the texture modifiers.
It's pretty much the same argument as the purple wood being added to Minecraft in the future - although their crimson planks are closer to purple than to the actual crimson color. That said, dark oak and spruce planks are fairly close in Minecraft - on low light (under 9) it can be difficult to be sure what you're looking at. The same goes for mangrove and acacia planks.
Anyway, this can be changed in the future if we get complaints.
There were no comments about the design process, but I made the bamboo textures based on our bamboo stalk, to give them a familiar look and to avoid design cues from Minecraft. I think it's best to keep our existing bamboo door.
This seems nice, although it seems to require experience with Blender, Blockbench, or similar. And as it makes it less trivial to update textures in the future, I don't know if it's worth the change.
I worked really hard on designing the new bamboo door, and prefer not to revert it back to the old one, but I could recolour it to match the MineClone 2 bamboo colours.
Does it? It just grabs the textures from the textures folder as usual, so I don't see anything difficult about changing said textures.
Why did you rework something that has been in the game about a month and has been through a review process for it to fit it with bamboo textures etc.?
How would you feel if in 2 weeks, I reworked your stuff?
To be frank with you, I only designed a new door because I had no idea which direction was the correct one with the old door. The old door texture looked way too ambiguous, and it confused me.
Besides, I would like to hear the opinion of @kneekoo on my proposal first. If he doesn't like it, then I can easily change it back.
That's what the Minetest wiki says:
as well as:
So the difficulty is not about the textures, but having to deal with Blender or similar software.
The door knob stands out very well, with better contrast compared to your version. So I don't understand the confusion about its direction.
My opinion was clear all along. Our existing bamboo door:
That's why I prefer it; not because I made it, but because the proposed version doesn't check any of the boxes that we want for the door. And we appreciate other everyone's hard work, but don't assume others haven't worked hard for what we have. So when you propose something, it needs to be an improvement.
I don't think the door models are going to look different anytime soon, so that would just be a one-off scenario.
What about now? I have now made the door match the style, not resemble Minecraft, and less boring.
You used the current texture and plugged in the cutout corners that I previously told you resembled like Minecraft's. Just leave the door as it is. You're wasting time on a non-issue, and it's not just your time.
Meanwhile, nothing has been done here to address the big issue - the looks of the door sides. Either we go with meshes, or we need the side textures back in, unless there's another way to deal with it.
There, no more Minecraft-resembling cutout corners.
I am leaning on the side of meshes. I think meshes have many advantages over nodeboxes, and lessening the amount of required textures is one of them.
You didn't leave the door as I asked. Please do so.
So be it. I only changed those for the reasons mentioned before and because I wanted to change every door in some way, shape, or form for this pull request. Not my problem anymore.
Hopefully meshes can be discussed now.
That discussion usually happens in an issue, not a PR.
I'm still not sure why reducing textures is so critical. We haven't had a 56k modem in decades. Most countries use Instagram, tik tok and image/video heavy content. A strip of 4 pixels, is this really a massive issue when it is cached on device? Plus, increasing complexity and reducing number of people that can work on and comprehend it is a cost not often discussed.
MineClone2/MineClone2#3372
MineClone2/MineClone2#3495
Not in the way one may think.
While having extra textures is not that big of a deal in the way of storage, it is a big deal in other departments.
Which is why I am decreasing the complexity by using meshes for the doors?
I don't see where this argument is coming from that using meshes for the doors will somehow make everything more difficult? Am I missing something? Is there a misunderstanding somewhere?
I've read through that discussion multiple times. Cannot see a word about side door textures.
I haven't seen anything in your main comment about texture packs.
I know nothing about the conversion process or what makes it more complex.
You need to be explicit what you're doing and why.
People aren't opposed to these changes if it helps TPs but half the time we have to read your mind or guess why you are doing something. Some times it feels like this is purposefully obfuscated.
To do textures OOTB, you have textures in order for main, top and side. They are often named as such. People can understand it without modelling experience. Some textures such as item stands and fences, certain pictures are used for different things and it isn't always obvious from what I've been told. If we move to where there is one texture, with different areas used for different parts, you are making it a requirement to understand the models and or picture structure to change and most won't get involved.
You only focus on complexity for tp conversion, tell us nothing about it then expect us to empathise with that process while refusing to put yourself in the shoes of other devs rather than factoring it in. It seems to put us in a position that we have to choose whose experience is worse because there is little attempt to bridge or find common ground. In a nutshell, you make our lives and your life harder in doing this.
Please help us help you.
If you are telling me that modelling knowledge and meshes are less complex that blocks of up to 16 pixels called top, side etc. then I'm a little baffled.
It's really not that complicated. My reasoning behind using meshes instead of nodeboxes for the doors is almost exactly the same as my reasoning behind using meshes instead of nodeboxes for beds as described here: MineClone2/MineClone2#2706 and for cocoa pods as described here: MineClone2/MineClone2#2974 just without the "because MC does it" part now.
In fact, the trapdoors also should get the same treatment of lowering the amount of unnecessary textures. Meshes should be used over nodeboxes for anything where, in the case that a nodebox would instead have been used, multiple textures would have then been required.
Probably how I felt. Just saying.
Door Texturing Fixesto Door FixesI don't think they're unneccessary textures. They are the side textures. This is done how Minetest advocates it.
You've not really made a case for why trapdoors need to go the same way other than making your life easier in terms of texture pack conversion. Can you not generate side textures from the original textures via a converter?
I'm not exactly happy with this PR and how there are now 8 models... 8! That feels ridiculously excessive for a door which essentially a cube. You haven't even said why...
The only reason this PR isn't dead is because the mirrored door seems to work straight away and correctly with redstone. Of course, this seems an overly complex solution to the problem. I am a big advocate of keeping things simple and non complex unless justifiable. I'm struggling with the justification.
The fact you're going to continue this process of converting to meshes for others makes me even more apprehensive. I don't think I'm going to entertain another PR mesh conversion PR unless you make a case for it through an issue that is agreed to. With this one, you pretty much ignored comments about meshes needing a discussion in an issue before we consider it. Part of me thinks if I merge this, you're just going to keep doing your own thing regardless. I have been extra patient over a number of your PRs as I think the first 2 big ones got a particularly rough ride (cocoa pods, then from a code perspective, grass), but that additional goodwill has probably been used up.
As I said, the only thing keeping this alive is you've solved the door mirroring problem. My question for myself is weighing up does the positives outweigh the negatives.
I am changing this now.
Door Fixesto Door Fixes & Improvementsb1ba6a91ec94eb43845cJust a note - for this, you need 4 doors total, because the door is inset rather than centered. Foss, you could do them as a 2 unit high mesh, with an atlas'd texture and then put in an attached node (like how the nodebox version of the doors are) that uses the transparent texture to act as the upper node. This would make it a better option than the nodebox version. You'll want to make a UV Map of the meshes for future devs as they will not know what part of the textures to adjust to create new content. (Unless the end texture was obvious...)
That would be the optimal way to do this... and it would be an improvement over the nodebox version. Though, your atlases might be larger filesize wise.
As far as justification:
It would save the engine from having to create a model from the nodebox definitions, and visually would move smoother than attached nodeboxes would.
Additionally, it would speed up the GPU's ability to texture all of the door's sides, and would only have a single-image material rather a cubemapped material like the nodeboxes have. That would allow the engine to use a faster shader. Note that cubemapped materials always uses 6 textures regardless of the number of textures that are defined in the Lua code. Hopefully, the minetest devs don't create copies when doing the material mapping.
I'm not 100% up on the minutia of how graphic cards handle the textures in memory in regards what it chooses to remove when the memory gets filled, but I think I recall reading that this was actually a more optimal way to load textures into the GPU and have them stay in memory, unless your atlas texture is super huge.
IE, it's saving the overhead of the individual textures' headers & pointer table that the graphics card has to reference each time it maps the said textures... allowing for more to stay in memory and less transfer from CPU to GPU. (Which is the major bottleneck for texturing.)
Hopefully, this will help and it can be accepted.
I have already replaced the meshes back with nodeboxes and multiple textures, but thanks for the tips anyway. I will try keeping those in mind for in the future.
Too bad. I'd like to see them be optimized meshes. A few less triangles in the draw calls 😃
This PR is a little in limbo. I will be completely honest 76 commits fill me with dread. By using an interactive rebase, I could easily fixup this branch and drop 44 commits without even thinking (grouping deletes and new textures). Part of me was tempted to squash this whole branch into 1 commit, but I didn't want to lose the models in case there was conversation later that convinced me of their usage. I am happy to recreate a rebased branch if you don't object (post-release potentially).
I would urge you to start working from local and using git (either commands or GUI). I will answer any questions, regardless of whether they seem silly or not. I am happy to try and see if I can clean up and fix the number of commits in this branch this time, but I do not want to do this again as it's time consuming.
I appreciate I gave feedback and you went ahead and made chances, but it did surprise me a little. I was expecting a conversation rather than changes.
I was hoping here you would elaborate one why. You understand the code and the problems as you worked on it. I do not. Explaining why you did something helps me understand the challenge.
I was hoping here you could help me by giving reasoning.
I was hoping you would create an issue to discuss meshes and why.
This was me thinking out loud and I was hoping you could help the positives outweigh the negatives through conversation. I'm not sure if you thought I was completely refusing models (I wasn't, I was just struggling to understand why there was 8 instead of 4).
Either way, what is done is done.
I was wondering what toppart and bottompart were and what issues you faced that made them neccessary (this is a question for understanding, not criticism).
Why is this needed? Is there something wrong with just creating a squash commit?
People should not be suffering through another month or two of broken door mirroring. This should get merged before the next release.
I did not want to waste any more time on arguing, so I just changed things back into nodeboxes for the time being. I find that the backend stuff can wait for later.
My goal originally was to make the door textures not require manual mirroring in order to look normal in-game, and the mirrored door fixes were just something that I managed to do while waiting for this to get reviewed.
Different meshes for both the top and bottom of the doors was simply because if I had just used the same mesh for all parts, you could probably see some weird texturing inside of the doors.
With some clever thinking, I could have potentially lowered the amount of meshes down to 2 or even 1, but this would have taken a lot of time.
There are 111 files changed in this commit. That is a LOT of files. I have not made a decision about whether this or the models is better, or better than what is in master (from a functional, complexity and quality perspective). All have their downsides and I need to time to reflect on things. If I squash commits, I cannot go back to previous commits and see the state, we also lose the models, and I don't want to lose stuff you've worked on unnecessarily. I personally hate squash commits, because some commits are essential, but 70+ isn't. I cannot see more than 3-10 in most tickets.
We have 750 issues. I have a bucket of things I want to look at before the next release, see the milestone for 0.83. Minetest 5.7 will release any day now (19th was suggested but delayed). We have to be ready to move if it goes ahead and we have bugs and we need to release. Also some of these tickets in the release bucket such as mob speeds are horrific for early game players. I have to weigh up whether I'm reviewing or fixing stuff myself. With all due respect, this issue is a low priority annoying bug. The only reason I weigh it higher is because people care about it. Suffering over this is small beans compared to some of the other bugs we have at the moment.
Ok, I will have to check it out and see if it justifies those extra textures.
I appreciate that, but it also makes my decision harder. I have to chose between meshes, this, or keeping what we have in master. Every PR I weigh up and see if overall it is an improvement.
I ultimately will ensure my focus is on the next release.
What does that mean? Everything becomes one commit and therefore makes reverting certain things more difficult? Is that the problem?
I don't really care about those models anymore. They were behaving very oddly anyways.
What does that even mean? This sounds like you meant something else entirely than what I had in mind.
So the misunderstanding here was that I thought that you were talking about the door meshes, and not the extra textures that I added.
Turns out that you were actually talking about the extra textures for the top part of the door top, the bottom part of the door bottoms, and the side textures?
Those extra textures are because nodeboxes use some very odd texturing method where you cannot rotate them without also warping certain textures. These extra textures are needed in order to open and close these doors without warping the textures.
I will say that, the use of meshes for something like this is a good idea. From an engine perspective, every nodebox node is a mesh that the engine has to make. From a graphics perspective, nodeboxes have 2x the triangles as a complete door mesh that is 2 nodes tall. From a visible/visual perspective, doing a 2x1 node mesh would remove the line across the middle of the door(s), as it wouldn't have two textures mapped across it. Additionally on this note, the door ends could be properly mapped and textured, so that it doesn't have weird artifacts on them. All in all,it would visually look better, and could be done to where the doors match up correctly.
One of the things that I can say with certainty is that these "little, inconsequential graphics annoyances" really do mean a lot to the players, and their overall satisfaction with the game. I'm not (at all) downplaying gameplay issues, as those are very important. But, one of the main things that has kept MC so popular is what is termed "builds". My friends actually go out, mine/dig/harvest thousands of nodes to do their builds. Like, one of them went and grabbed 3200 blocks of sand just for glass. I, myself, am 1/20th of the way for the idea I have for my build, and I have grabbed a chest full of magma cubes, sand, gravel, ink sacs. (Note: that's a chest for each... not one chest with those items.) I built a huge automated Production Area to smelt things, and to make concrete, etc. And, people choose things based on how they look together.
Additionally, I watched a video of a build that a person put over 500 hours into, that looked phenomenal. And, it's things like that, that drive the gameplay for this game. Players want to make cool things, and this gives them a shared platform to do so, without it costing them to buy the blocks that they want for what they want to do.
I wish to discuss this after this pull request has been merged, since I just want to get the glaring problems out of the way first. Those being the messed up door texturing and the double doors being stubborn.
As it currently stands, merging this pull request fixes every in-game issue regarding doors.
Feel free to squash. I don't care anymore about those meshes. They were behaving very weirdly anyways.
It makes reverting commits impossible and has to be manually done. Also if you add a file in, and delete it, it vanishes completely.
Ok.
I haven't reviewed this yet, as it's massive and daunting. A 110 file PR is not easy to review and takes time.
Can they not be rotated? When you open the door, does it only show the top could of pixels and not the rest?
Interesting points.
I'm not discounted that people find them important. We have 750 issues, and about 5-7 developers. Unless we're all working on 100 issues each at the same time, we have to prioritise. If everything is a priority, nothing is a priority. I don't know about you, but I can only focus on 1 thing at once. I have to ruthless because every time a shift my focus to another issue, one gets dropped down the list.
I am not saying this isn't important, but I am saying there are more important issues at play. What is the point in having mirrored doors if people cannot appreciate them because they didn't get past the start because they got slaughtered in the dark because mobs were too fast for them? In that case, they've probably already quit to go play MTG.
It's impossible to get the texturing right on nodeboxes without creating these extra textures, or using certain texture modifiers which break compatibility with larger resolutions.
It's difficult to explain, but let's just say that nodeboxes get textured as if they were a full node. That means that the rest of a texture is actually still there outside of the nodebox, it just isn't visible.
You can do texture transforming, but Foss is correct in RE to how the nodeboxes work with textures.
To explain it... I would say to think of having a cube made from the textures. So, you have six sides... and, your node definition is inside of that cube. only the parts of the nodebox that line up (area wise) in the node get the texture. The rest of the texture is there, but isn't shown. So, in rotating the door, by simply changing the nodebox definition (the curly braces, not the entire node def) it's taking different slices of the textures and slapping them on the created mesh. It's a projection of the textures. Kind of like... if you projected light (with a picture) at all six sides of a door in space, and then rotate the door 90 degrees... the pictures projected onto the door is significantly different in its two states.
With a real mesh object, however, the texturing would be the same regardless of its rotation. Which, is another point as to why it's a better idea to mesh it out.
Also, I wasn't saying to make this a high priority... I was just offering that it should have a different mental priority than "oh, that's just cosmetic." I get that you have a billion things on the todo list, and that you have to be very selective... With game-breaking issues and "wow, this totally sucks!" issues taking the forefront, as they should. Those are base gameplay issues, while this one is a player retention issue. Just like how the farms are.
@ancientmarinerdev Please consider merging this. I'd argue that the positives far outweigh the negatives.
If the improvements in mechanics were separated from the looks, in different PRs, it would make it easier to merge them and work on the looks later. As a whole, this is one step forward and one step back.
I cannot merge something until I'm happy with it. It's with a heavy heart that I haven't yet merged it, but I just don't know where to start with this. It's really hard to review. I'm not surprised that no one else has picked it up neither.
If things want to be reviewed they need to be kept as small as they can be, and the scope needs to be small.
As Nicu has said, there is look and feel changes and code changes. I have no idea what change was done for what reason, and with 76 commits, it's not like I can see the changes and link to a commit.
I will re-review the code again, but if I can get an itemised list of what textures have been added/changed and why, it will help to understand.
If I remember correctly, the code changes to a reasonable extent seemed logical, but I'll have to rewrap my head around it.
The majority of those commits are of me just adding and removing textures the entire time, but I will try telling all the positives and negatives of this pull request.
POSITIVES:
NEGATIVES:
The negatives are negatable, because it's not as if we are creating a new door type every day.
Modder headache though.
I retested this, to check its state. The textures don't look correctly in the cases I mentioned previously (oak, acacia, iron, jungle). They're cut from the full texture of the door. That also makes the sides look like they don't move at all.
Considering the issues above, if we can easily customize the doors to look properly, that's a win. But I don't see how we can do it without designated textures for all sides.
The issues and the negatives can't be dismissed. Some people use texture packs and that's where it matters the most. It also matters when we introduce new wood (cherry for now).
Why do they have an extra top side and bottom side? Why is this not the same image?
They don't, that's the thing. It's just a partial texture of the full one, and asymmetric textures will look bad on the top, bottom, and sides.
That's on purpose. Door hinges don't exactly move around with the doors, do they? They remain stationary and always at the same spot.
But there are textures for all sides?
With some texture pack converter improvements, the texture pack issue can be easily resolved.
Introducing new wood does indeed become more tedious, but this seems like a slight inconvenience which doesn't even happen very often.
Unlike meshes, it's difficult to do very specific texturing on nodeboxes, so extra textures are needed.
The textures don't look wrong to me. They seem perfectly fine.
They are supposed to look differently, not pixel-by-pixel identical. That's a visual bug.
Also, hinges are not the same thing as the door sides.
Yes. Check the doors in 0.83.
We need to make it easy/easier, not more tedious. Saving a few hundred bytes is not worth losing easy modding/additions.
As shown in my screenshot, the texture on top is cut - it looks bad.
That seems to be an issue specific to the doors of those wood types, then?
Yes: acacia, iron and jungle wood.
But I just checked in Minecraft and they way you did it is just like they do it. They only have two textures (top+bottom), and all sides are generated from those 16x16 textures. And their doors are actually uglier than ours, considering we only have 3 that looks weird on the top and bottom sides.
If we want to stick with how Minecraft does it, we're good.
For better looking doors, I think we should support 2 extra textures - one for the top/bottom sides, and one for the lateral sides. If they're not defined, just do what Minecraft does. What do you guys think? Of course, this is now a feature request.
When the doors were still using meshes in this pull request, that is exactly how I made the door textures work. I was forced to change this after complaints about the usage of meshes.
However, since texturing nodeboxes is not as streamlined as texturing meshes is, I needed to use more textures in order to get the doors textured in exactly the same way as before the usage of nodeboxes.
I indeed prefer to do exactly that.
Is what you are requesting even possible with the current engine? I don't think that you can have an "if x then y else z" when registering node textures.
No. That isn't what happened.
"I'm not exactly happy with this PR and how there are now 8 models... 8! That feels ridiculously excessive for a door which essentially a cube. You haven't even said why..."
Me stating you haven't said why it needs it, only needed you to answer why you did it, not redo everything. You work on something and understand it well, I don't. You need to communicate the reasoning behind decisions so I cannot understand it.
What do you mean by streamlined?
We copy gameplay, not look and feel or architectural decisions. We work on a different engine to them, and their decisions on their engine make sense to that context, we need our decisions to make sense in this context.
Node registrations can happen in functions where you pass parameters in. There can be conditional blocks in the functions or in the logic that calls functions. Most things programmatically is possible. It's just seeing how it can be done cleanly and logically.
It's because in order to get the doors to look the way I wanted them to look, without the usage of any more than one texture, I would have needed to use meshes for this.
I am not the best when it comes to meshes, and I made a separate mesh for each of the following:
2^3 = 8
I could have probably lowered the amount of meshes used significantly, but I did not want to do that in this pull request, instead wanting to improve upon this later in a separate pull request once my meshing skills have gotten better.
Easy to work with.
This assumes that aesthetics aren't part of the gameplay, which I disagree with. The difference with Minecraft as well is that they can actually change the engine to their liking, which we can't (unless we want to go through the effort to create a fork of the Minetest engine specifically made for MineClone 2).
I still find it hard to believe that you can register nodes on the fly during gameplay.
I've reviewed this from a code perspective. I would like Nicu to review the asset side of things.
@ -668,3 +668,2 @@_mcl_blast_resistance = 3,tiles_bottom = {"mcl_crimson_crimson_door_bottom.png", "mcl_doors_door_crimson_side_lower.png"},tiles_top = {"mcl_crimson_crimson_door_top.png", "mcl_doors_door_crimson_side_upper.png"},tiles_bottom = "mcl_crimson_crimson_door_bottom.png",Why do we have sides lower down, but not here?
I don't understand the question?
Why did you remove the door side here, but they are listed further down in other nodes?
Explicitly mentioning the door side textures is no longer required, since this is now done automatically thanks to the
def.tiles_top:match("(.+)%..+$")code that you asked about afterwards.@ -204,3 +204,2 @@local tt = def.tiles_toplocal tb = def.tiles_bottomlocal doortextop = def.tiles_top:match("(.+)%..+$")What does this part do?
This removes the filename extensions from the textures.
@ -207,0 +205,4 @@local doortextop = def.tiles_top:match("(.+)%..+$")local doortexbottom = def.tiles_bottom:match("(.+)%..+$")local tt = doortextop .. ".png"These variable names are dreadful. I know originally this was a sin of the past, but don't copy people doing the wrong thing. It's one thing to work on a bad codebase. It's another thing to consciously keep it bad. :)
tex_top
tex_bottom
tex_under
tex_over
would have been far preferable if you must go for short variable names. At least then at a glance, the person reading it would know what it is without having to scroll lots of times.
I'd personally go:
texture_panel_bottom
texture_panel_top
texture_bottom_side
texture_top_side
texture_top
texture_bottom
The computer won't penalize you for long variable names and the person who next comes into the code will thank you.
I have changed some names now to be clearer.
@ -207,0 +207,4 @@local tt = doortextop .. ".png"local tb = doortexbottom .. ".png"local ttt = doortextop .. "_toppart.png" -- Special texture to make the top of opened doors not look weird.Why can the toppart and bottompart not be the same image? What cases would justify a separate line of different pixels?
The same reason why there are separate images for the sides of the door top and the door bottom each.
Which is?
Because of the usage of nodeboxes, opening a door basically flips the nodebox around without actually flipping around the textures as well, which makes the textures look strange. Using extra textures is a simple band-aid workaround.
@ -207,0 +209,4 @@local tb = doortexbottom .. ".png"local ttt = doortextop .. "_toppart.png" -- Special texture to make the top of opened doors not look weird.local tbb = doortexbottom .. "_bottompart.png" -- Special texture to make the bottom of opened doors not look weird.local tts = doortextop .. "_side.png" -- Special texture to make the side of opened doors not look weird.Do the top and bottom side look different?
Yes, they are different. it's just that the PixelPerfection textures have the sides look barely any different to be noticeable, but with other texture packs, the differences are much more visible.
@ -279,3 +306,2 @@minetest.register_node(name.."_b_1", {tiles = {"blank.png", tt[2].."^[transformFXR90", tb[2], tb[2].."^[transformFX", tb[1], tb[1].."^[transformFX"},minetest.register_node(name.."_b_1", {We use tab spaces. You removed a valid tab space, and replaced with spaces. If it's your editor doing that, you need to configure it to not do so.
I didn't notice that. Will be fixed.
@ -351,3 +378,3 @@minetest.register_node(name.."_t_1", {tiles = {tt[2].."^[transformR90", "blank.png", tt[2], tt[2].."^[transformFX", tt[1], tt[1].."^[transformFX"},tiles = {ttt .. "^[transformFY", "blank.png", tts, ttsm, ttm, tt},Better naming would have made this line less of a WTF?!
I'm on it.
@ -423,3 +450,3 @@minetest.register_node(name.."_b_2", {tiles = {"blank.png", tt[2].."^[transformFXR90", tb[2].."^[transformI", tb[2].."^[transformFX", tb[1].."^[transformFX", tb[1]},tiles = {"blank.png", tbbm, tbs, tbs, tb, tbm},Better naming would have made this line less of a WTF?!
@ -495,3 +521,3 @@minetest.register_node(name.."_t_2", {tiles = {tt[2].."^[transformR90", "blank.png", tt[2].."^[transformI", tt[2].."^[transformFX", tt[1].."^[transformFX", tt[1]},tiles = {tttm, "blank.png", tts, tts, tt, ttm},It looks the right idea to create these new nodes, however, if many of these properties were the same, it was probably better to create the definition once. Register it, change only the bits that need to change, and register that bit.
Duplicate code is a code smell. Copy paste coding is bad idea in general. Only thing I ever copy paste is variable names. I cut paste more than I copy paste. I might temporarily copy code to change it before I delete the old one.
@ -498,0 +569,4 @@local bottom = {x=top.x,y=top.y-1,z=top.z}local meta_bottom = minetest_get_meta(bottom)meta_bottom:set_int("rotation", 1)node.name = name .."_b_2"rotated_node = name .. "b2",
and in this function, you can then do sonething like:
node.name = rotated_node
and all those 4 functions are the same and 3 of those duplicated code could be deleted.
Did my code improvement commit 4 days ago resolve this now?
@ -498,0 +591,4 @@minetest.register_node(name.."_b_3", {tiles = {"blank.png", tbm .. "^[transformFY", tbs, tbsm, tb, tbm},use_texture_alpha = minetest.features.use_texture_alpha_string_modes and "clip" or true,paramtype = "light",Most of these definitions are duplicate code. Hard to review, hard to maintain.
@ -525,0 +733,4 @@endminetest.register_node(name.."_b_4", {tiles = {"blank.png", tbb, tbsm, tbsm, tbm, tb},Most of these definitions are duplicate code. Hard to review, hard to maintain.
As far as I can tell, we're in good shape with the assets. We just need the origin and licensing mentioned for the newly added images, to be sure we're in good condition to distribute them.
All of them are either straight copies or derivate works of either the PixelPerfection or the MineClone 2 textures.
So everything has already been credited.
I have changed the code quite a bit now. I have also closed some conversations which I assume have been resolved now. Feel free to open them again if these have been falsely marked as resolved.
Thanks for taking the feedback on board. I think I'm happy to approve from a code perspective.
I have discussed with Nicu, and one thing we're both not keen on is the top of the jungle door (toppart, I think). The fact there are holes in it makes it look hollow which isn't a good look.
Acacia is a biiig improvement. I think the textures are looking pretty sweet.
The holes by themselves make sense because the door in general is supposed to look shabbily made in PixelPerfection. But if you so insist, then fine by me.
I have also closed the bottompart holes since those would have probably made the door look hollow as well, even though you wouldn't exactly see those textures often.
I don't mind the bottom part, but at the top, those holes need to go. Looks like 6 pixels on the top. And 6 on the top mirrored part, and 6 on the mirrored toppart (i'm not sure why those holes are there if they went in the non-mirrored version).
If the top is fixed, the other holes will still have the effect.
I just tested this again.
And because the most recent commit claims to have fixed the holes, I wondered if those files are even used by the game, so I renamed them and restarted:
Clearly they are required, but it looks like they are not used. Also, this looks like you didn't test the doors after you changed the textures.
But considering the top and bottom textures are not even applied, why do we even have them? It's like either our code, or the engine, applies the top/bottom sides, then overwrites them using the top/bottom sides of the two top/bottom door halves. If this is what happens, the engine is clearly capable of using the top/bottom halves for the top/door sides, and we don't need extra textures.
There, now the jungle door tops have had the holes closed.
All of the textures do get used at least once, and a look at the code confirms this.
should we also do the topparts and bottom parts for the cherry blossom door once it's ready?
Apologies for the delay in testing and getting back to you. I've had a busy few weeks.
I have tested this, and just confirmed what Nicu raised. I knew it was an issue when I glanced at his screenshot but had to verify first.
When someone gives you feedback, don't get defensive, we're trying to get things right and over the line. By not listening, and not verifying, you waste mine and Nicu's time, because we had to test and now we have to test again, until it's done. If you test your changes, and listen to feedback, you save us all time and we get this over the line faster. Also, as this isn't the first time, and after a while, I just do not enjoy reviewing your PRs. I expect I'll give feedback, you will get defensive and argue. I can't merge it and you won't change it and it'll just sit there for longer. As a result, I take longer to get to it. This isn't my job, it's a hobby, and I absolutely do not enjoy arguing and debating online, it just burns the energy I have for the day. If you want a quick review, please consider reviewer/testers time.
I swapped Crimson and Warped top part. The toppart is only used for the toppart and not the mirrored toppart. It looks like you mirrored the top for the mirrored toppart rather than mirroring the toppart as you can see the 3 dark pixels at the side from the pattern. So it isn't even used in that context.
Seems to be better for open doors.
There, I fixed it now?
The way @kneekoo worded the issue was too confusing for me, it didn't help either that a browser extension which I had on, had broken the embedded Imgur attachments.
If you can't be bothered to test this trivial thing, and you have to ask, then I have no interest in trying this branch again. Other people's time is important too.
Know your extensions, and what they can break, so people don't assume you can see the images when you don't. This is especially important when we spend time to test and give feedback.
I always add descriptions to my screenshots, so if the images didn't load, you had those. If your extension removed even those, then use something that does a good job.
I will test it if you want
works fine on my end
My fault for being vague when I said that, but I did test it out and everything seems to work just fine. I am just unsure whether or not that was everything or not.
Hi Foss, Retested this and the toppart textures seem to work as expected now, and the jungle door doesn't look hollow anymore. So those issues are resolved. We're pretty close now. Just noticed we have 11 textures with alt in the name that don't appear to be used, for example:
mcl_crimson_warped_door_top_alt_toppart
Are these no longer needed?
But why are the top colours flipped between doors? That doesn't make sense.
I swapped to test the textures were coming from the right place.
These
alttextures of the warped doors are meant to be for anyone who wants to use the teal coloured warped doors that Minecraft has, instead of the purple ones currently being used.They would need to manually swap them out though. There is no reference to them in code, so they just take memory every time someone starts the game. Someone would have to manually and purposefully swap them. Wouldn't that be better as a texture pack?
I never knew that people didn't just manually swap those textures. I should probably remove these then.
those should be removed, and moved over to the official modpack. The mod pack, when I last touched it, had those set up properly... so they are not needed in the main game's textures.
Also, why are the texture deletions an individual commit, rather than just one commit?
I don't really touch anything outside of the main game, so someone else is free to add those textures to the modpack.
Since I find using the command-line too confusing, I use the Gitea GUI to do my commits. This comes with its limits, such as needing a separate commit for each individual texture removal.
So if someone knows of any good command-line Gitea tutorials or some sort of better GUI to commit things, feel free to suggest these.
Thanks for doing that and implementing feedback.
Merged as a squash commit.
Yeah, that has been implemented in the Official Modpack, as it's not mainstream game stuff. The Maintainers decided that it should go there, so that Mineclone can be different. So, I made a mod for it.
I usually use the built in git gui in IntelliJ. It works well for deletions as a single commit. (not the greatest tool on some things, but that, it works well.)
I found a program called
git-colawhile trying to do MineClone2/MineClone2#3758 and it seems to do the job well enough for me at the moment.