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

Changes to torch and delayer appearance #669

Merged
merged 9 commits into from
Jun 8, 2024

Conversation

mruncreative
Copy link
Contributor

Changed torch to be plantlike, x-shaped instead of just a sprite and only use 1 texture instead of 3 and changed delayer to require only 3 textures from texturepacks instead of 13 changing the model to be a simple slab. Yes, I'm sad for whoever put in the work into the delayer model but it clashes with texturepacks and requires way to much work. Especially if they are not 16x16.

…only use 1 texture instead of 3 and changed delayer to require only 3 textures from texturepacks instead of 13 changing the model to be a simple slab
…ers. Will continue to work fine with all existing texturepacks. No change in graphics.
Copy link
Member

@Niklp09 Niklp09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some comments from my side:

  • The new delayer textures look awful IMHO
  • Sharing some textures between luacontroller and microcontroller seems fine
  • Sharing piston textures seems fine too
  • The new torch style looks very nice

screenshot_20240420_091742

@mruncreative
Copy link
Contributor Author

mruncreative commented Apr 21, 2024

grafik
Would you approve of this? Now it only requires 1 additional texture for texture pack creators to support the delayer after supporting the rest of the mod.

@mruncreative
Copy link
Contributor Author

Before anyone asks: License of my arrow of slowness and code changes is CC0

@Niklp09
Copy link
Member

Niklp09 commented Apr 23, 2024

License of my arrow of slowness and code changes is CC0

Not sure how the license situation here is, acording to the readme all media files are cc-by-sa 3.0.

@mruncreative
Copy link
Contributor Author

mruncreative commented Apr 23, 2024

I said CC0 meaning I don't give a shit what you do with it or how you relicense it.
I would like to know what you think of the new delayer and what still needs to be done until it can be merged.

…ing the digistuff mod which inherits it. I hope no mods inherit microcontroller LED.
mesecons_gates/mod.conf Outdated Show resolved Hide resolved
@Niklp09
Copy link
Member

Niklp09 commented Apr 26, 2024

I would like to know what you think of the new delayer and what still needs to be done

I like the "old" delayer design as it looks (more) like an arrow, this is pretty subjective.

@mruncreative
Copy link
Contributor Author

If no one cares anyway, why not just merge?

@Niklp09
Copy link
Member

Niklp09 commented May 11, 2024

why not just merge?

Because no one approved your changes, I recommend to move the uncontroversial stuff to a second PR.

If no one cares anyway

Probably due to your previous behavior...

@mruncreative
Copy link
Contributor Author

mruncreative commented May 12, 2024

This PR has been up for a month now. I asked if anybody would like to volunteer reviewing this on IRC two days ago but nobody even replied.
https://irc.minetest.net/minetest/2024-05-10

I really think the people who are active don't care and those who aren't active I can't ask either way. I don't see how it's a controversial change. It doesn't break any other texture packs or mods to my knowledge except for the top of delayer in existing texture packs. However the old delayer is an ugly piece of shit, that requires 13 texture that are variations and the elevated voxels of the circle assume a 16x16 texture pack and look shit with all other resolutions like 32x32, 64x64, 128x128. Here is a picture from mesecons.net. I think it's the hdx-128 texture pack. It's objectively ugly. The other mesecons textures in the pack look better. I think the creator of the pack gave up on it and this monstrosity was born.
grafik

Of course I'm biased because I'm maintaining a 128x texture pack too.
https://content.minetest.net/packages/shaft/hand_painted_expanded/

I can change the symbol to a simple arrow if that pleases you.

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Things I like:

  1. rotated piston textures. Reduces texture count, which is nice.
  2. the torch looks much better now

Things I don't mind:

  1. Delayer nodebox. Unified design with gates. Fine by me. The old design looked good and a simpler version is not bad either.
  2. Delayer texture. Very simplistic, yet fits the other gates, thus OK.
    • EDIT: I'd welcome the design shown above with the straight arrow. I this this preserves the legacy better while still keeping the functionality of the gate visible (dashed arrow).

Things where I don't agree:

  1. jeija_gate_*.png should be kept in the mesecons_gates mod. These texture are too specific for the base mod. Perhaps make the delayer depend on gates?
  2. jeija_luacontroller_*.png are too specific for mesecons. But what would be a better place to put them?
  3. Delayer appearance: the output side is unlike the other gates. It should indicate whether the output is active (currently not visible).

@mruncreative
Copy link
Contributor Author

mruncreative commented May 24, 2024

Thank you so much for your review.

Things where I don't agree:

  1. I will add the dependency of gates on delayer back and put them in delayer. That seems like a good solution and you're probably right about that.
  2. They were already there and I see no reason to move them. The jeija_luacontroller_LED_*.png in luacontroller were removed, to reduce the amount of textures and then the jeija_microcontroller_LED_*.png were renamed to jeija_luacontroller_LED_*.png because digistuff depends on the luacontroller leds and I don't want to break it.
  3. I can fix that too.

@mruncreative
Copy link
Contributor Author

grafik
All done and I've changed the symbol to the bold arrow which you clearly both prefer.

@mruncreative
Copy link
Contributor Author

mruncreative commented May 24, 2024

It doesn't actually indicate whether the output really active because of the delay though which is why I originally didn't do it. Just points to where the output is.
(The power out of the old delayer doesn't either, so it's probably fine.)

Copy link
Member

@SmallJoker SmallJoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comparison

the jeija_microcontroller_LED_.png were renamed to jeija_luacontroller_LED_.png because digistuff depends on the luacontroller leds and I don't want to break it.

(partially a note to myself) This must be put into the final commit message to preserve the reason for this renaming.

Aside from the delayer, the other texture changes lead to no visible change with only the "base" texture pack being active, which is good. In the future, more contrast on the gate/delayer textures could make them easier to look at.

Fine by me. If there's any objections, please let me know. If there are none, I'll go ahead and merge this PR in a week.

mesecons_delayer/init.lua Outdated Show resolved Hide resolved
Copy link
Member

@Niklp09 Niklp09 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for improving the overall consistency of mesecons.

@mruncreative
Copy link
Contributor Author

TIME TO MERGE ☺

@SmallJoker SmallJoker merged commit 0a4651c into minetest-mods:master Jun 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants