search

minetest / irrlicht

Minetest's fork of Irrlicht
Other
115 stars 87 forks source link

Reformat the code #248

Closed numberZero closed 9 months ago

numberZero commented 1 year ago

This is a (hopefully final) prerequisite for minetest/minetest#13642.

I used .clang-format from Minetest but altered it a bit. In particular, clang-format isn’t all that good on wrapping long lines, so I removed line length limit to avoid ugly continuation.

Now, I don’t claim it is all pretty and nice now, but it’s much more consistent with both Minetest and itself.

numberZero commented 1 year ago

Reapplied.

numberZero commented 11 months ago

Reapplied. @Desour

numberZero commented 11 months ago

It's a bit sad that we're loosing the alignment.

Alignment? Oh wait, you’re likely still using a monospaced font. Okay...

How hard would it be to replace the "Strip non-leading tabs" commit with one that replaces all the tabs with the right amount of spaces?

Doing exactly what you suggest is possible but won’t help as great deal of these non-leading tabs wasn’t even legitimate alignment. Up to this sample in include/SMaterialLayer.h. Manually working through that is possible (it’s just 1k lines) but out of scope of this PR.

Saying that, now I think that commit wasn’t at all good, due to e.g. code samples in comments. I removed it from the branch as it would be much easier to reapply its good part than to undo its bad part later.

Desour commented 11 months ago

Oh wait, you’re likely still using a monospaced font. Okay...

Yes, I just find it's neat.

Manually working through that is possible (it’s just 1k lines) but out of scope of this PR.

I thought more about writing a script that does it.

Saying that, now I think that commit wasn’t at all good, due to e.g. code samples in comments. I removed it from the branch as it would be much easier to reapply its good part than to undo its bad part later.

Good solution for now. :+1:

numberZero commented 11 months ago

Reapplied. Yes, it has to be reapplied when pretty much any other PR is merged. For the obvious reason. Maybe it’s better to split it in parts?

sfan5 commented 11 months ago

I think we should do this as the very last step before importing it into the MT repo since at that point the ability to easily backport upstream commits and all PRs on this repo are lost anyway.

numberZero commented 11 months ago

What else holds the import, then?

Desour commented 10 months ago

It has been decided in the last dev meeting, that #276 will be merged first. After that one is merged, we won't trigger another rebase of this PR. See discussion starting here: https://irc.minetest.net/minetest-dev/2024-01-21#i_6147328

sfan5 commented 9 months ago

I think we can also use this opportunity to move all files from source/Irrlicht to just src. thoughts?

Desour commented 8 months ago

@numberZero I've seen you've deleted some of your fork repos. If you don't want to work on this anymore, we'll adopt this. :-) I hope you're doing fine though.

Desour commented 8 months ago

295

I think we can also use this opportunity to move all files from source/Irrlicht to just src. thoughts?

Included in new PR.