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

Docs: add "flip moon texture" into breakage file #12417

Merged
merged 4 commits into from
Aug 14, 2022

Conversation

Zughy
Copy link
Member

@Zughy Zughy commented Jun 8, 2022

As someone who both makes art and develop, the flipped moon still annoys me (yes, it is flipped if you've had to document it #11940). Considering the "controversial" tag in #11902, this should be an approach that makes everyone happy in the long run

To do

This PR is Ready for Review.

How to test

read

@Zughy Zughy added @ Client / Audiovisuals @ Documentation Improvements or additions to documentation Breaking Change Changes an existing API behavior in an incompatible way labels Jun 8, 2022
@sfan5 sfan5 removed Breaking Change Changes an existing API behavior in an incompatible way @ Client / Audiovisuals labels Jun 8, 2022
@appgurueu
Copy link
Contributor

yes, it is flipped if you've had to document it

This argument is moot. We would've had to document the orientation being the same as the sun's as well. To add to the never-ending chain of analogies: Consider XYZW vs WXYZ quaternion order. You have to document which one it is. You can't just say "because we've had to document XYZW we should WXYZ instead because it's obvious". IMO neither orientation is "obvious" or "more intuitive". Both would have to be documented.

@erlehmann
Copy link
Contributor

Considering the "controversial" tag in #11902, this should be an approach that makes everyone happy in the long run

Nope.

First, breaking every mod that actually cares about moon rotation does not magically become a good thing if documented.

Second, only you seem to think there is a “right” way up. Rotation is pretty much arbitrary; not breaking things is preferable.

Third, you already tried this once in #11902 and it was extensively discussed and ultimaltely rejected. So please cut the crap.

@Zughy
Copy link
Member Author

Zughy commented Jun 9, 2022

Second, only you seem to think there is a “right” way up.

imagen

For the rest of what he's said, I leave to the reader the pain to go through the PR both he and I mentioned, which it ended with "I think compatibility is most important here.", hence my 6.0 breaking suggestion

@appgurueu
Copy link
Contributor

appgurueu commented Jun 9, 2022

Second, only you seem to think there is a “right” way up.

imagen

Oh so we're going with popularity here? Guess what:

Bildschirmfoto von 2022-06-09 17-48-06

rubenwardy
rubenwardy previously approved these changes Aug 12, 2022
@corarona
Copy link
Contributor

once you have broken everything at least one time, will you consider making the world larger ? :(

@rubenwardy
Copy link
Member

Yes, that should be added to this file

Please note that this file is not binding, it's notes about things to consider when doing 6.0. I don't think the moon direction is important, either convention is fine, I just don't want to spend any more time talking about it

@appgurueu
Copy link
Contributor

appgurueu commented Aug 12, 2022

I don't think the moon direction is important, either convention is fine, I just don't want to spend any more time talking about it

Then close this PR. Simplest way to go about it: No (breaking) change is needed.

@rubenwardy
Copy link
Member

If anyone cares about this when we get to 6.0 then we can have the discussion then. I cba to go through this discussion again for such a tiny issue

@rubenwardy rubenwardy reopened this Aug 13, 2022
@rubenwardy
Copy link
Member

Actually, I'll just unsubscribe from this thread

@celeron55
Copy link
Member

I would be fine either way, so if any core dev wants this in I give them my approval.

Copy link
Member

@sfan5 sfan5 left a comment

Choose a reason for hiding this comment

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

<+sfan5> it will be reassesed anyway when the time actually comes so we can just add it to the file

@sfan5 sfan5 merged commit 760242c into minetest:master Aug 14, 2022
@Zughy Zughy deleted the breakage-moon branch June 4, 2023 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Documentation Improvements or additions to documentation >= Two approvals ✅ ✅
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants