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

Introduction of a Hexagon Cell Grid in the GridMap node #822

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Wasabi-Cheetah
Copy link

@Wasabi-Cheetah Wasabi-Cheetah commented Oct 29, 2024

I've merged the work done by BSChad in his fork of Godot.
It Introduces a new Hexagon Grid option for the GridMap node.
Proper accreditation given in each file since I'm just a merger.

Locations of files changed for Hexagon feature in GridMap node.

redot/core/math/math_defs.h
modules/gridmap/doc_classes/GridMap.xml
modules/gridmap/editor/grid_map_editor_plugin.cpp
modules/gridmap/editor/grid_map_editor_plugin.h
modules/gridmap/grid_map.cpp

Since I'm on Linux, I will do build for Linux and post it on my fork.

Related: godotengine/godot-proposals#4337 and godotengine/godot#85890

Changed some license header details and added a float value
Implemented core scripts for defining a hexagon cell as well as include the option to switch to hexagon from grid in the GridMap options.
From the developer's notes at line 321: General scaling and translation for the cell mesh.  The translation is necessary because the meshes are centered around the origin, and we need to shift the cell up.
From what I can understand. updates the file to include grid changes and introduce the hex grid plus corresponding  directions and caching for transforms.
From inline dev notes, Introduces the changes to rotating and flipping hexes so Y is down, crucial component for 3D art as well as other notable changes.
From dev notes - SQRT(3)/2; used both in the editor and the GridMap.  Due to the division, it didn't fit the pattern of other Math_SQRTN defines, so I'm putting it here.
changed the top line + Introduction of the hexagon cells into a hex grid floor, adding transform options like rotation and flipping for cells and more.
Edited second line
Changed top lines 1 and 2 + Introduced hexagon cells into the core GridMap node, including coordinates for diagonal directions.
@@ -41,6 +41,7 @@

#define Math_SQRT12 0.7071067811865475244008443621048490
#define Math_SQRT2 1.4142135623730950488016887242
#define Math_SQRT3 1.7320508075688772935274463415059
Copy link
Member

Choose a reason for hiding this comment

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

This is a trivial math change, it does not need attribution.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, thanks for letting me know, wanted to be thorough.

Copy link

@rlidwka rlidwka left a comment

Choose a reason for hiding this comment

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

I believe this merge is done completely wrong. If you're using git for the first time, that's understandable I suppose.

Original work is presumably here?
https://github.com/BSChad/godot/commits/feat-grid-map-hexagonal-cells/

How it is now: All commits are from you. Each file is its own commit. You're giving credits in the file contents.

How it should be: Commit metadata should be exactly the same as original. There should be no changes in the headers at all. Attribution should be given in commit messages.

I'd suggest to use git cherry-pick.

@Wasabi-Cheetah
Copy link
Author

I believe this merge is done completely wrong. If you're using git for the first time, that's understandable I suppose.

Original work is presumably here? https://github.com/BSChad/godot/commits/feat-grid-map-hexagonal-cells/

Yeah first time, and yes that's the repo.
Learning as I go about all this of course.
I'll look into that for sure.

@rlidwka
Copy link

rlidwka commented Oct 29, 2024

also, this is an open PR in Godot (85890), which they want to release as 4.4

just wait until it appears upstream

Redot should be merging old and/or rejected PRs, not the new ones that would appear in Godot anyway

@Wasabi-Cheetah
Copy link
Author

It's been ready to merge for months
so it seemed ignored by the higher ups,
thought I'd take a stab at it and merge it.
Helped me learn about proper merging.

@rlidwka
Copy link

rlidwka commented Oct 30, 2024

Okay, here's how you merge stuff:

git clone git@github.com:Wasabi-Cheetah/redot-engine.git
cd redot-engine
git checkout -b feat-grid-map-hexagonal-cells
git fetch https://github.com/Redot-Engine/redot-engine master
git reset --hard FETCH_HEAD
git fetch https://github.com/BSChad/godot feat-grid-map-hexagonal-cells
git merge FETCH_HEAD
git push git@github.com:Wasabi-Cheetah/redot-engine.git feat-grid-map-hexagonal-cells

Then compile the engine and verify that all of it works as expected. That's the most important part, without which your PR has no value (since the commands above can be run automatically and by anyone).

I've just did that, and here's the changes you should have in the PR:
77eaec7...rlidwka:redot-engine:feat-grid-map-hexagonal-cells

@Spartan322 Spartan322 added the tracked:godot Issue already tracker on the godot issue tracker label Oct 30, 2024
@Spartan322
Copy link
Member

Also could I get the related Godot PR/proposal for this?

@Wasabi-Cheetah
Copy link
Author

Also could I get the related Godot PR/proposal for this?

I opened a discussion using the redot-proposals repo here.
Redot-Engine/redot-proposals#41

It contains the reference PRs from Godot by BSChad and octetdev2
I don't know if I did that right since I'm new to making proposals.

@Spartan322
Copy link
Member

@Wasabi-Cheetah
Copy link
Author

Related: godotengine/godot-proposals#4337 and godotengine/godot#85890

Forgot to add those important links, added it to my first post, thanks.

@Spartan322
Copy link
Member

Spartan322 commented Oct 30, 2024

Personally I'd prefer to wait on the Godot PR as its still being actively developed, its not abandoned nor stalled nor rejected by Godot, it had another commit just last week and it does not appear to be finished.

@Wasabi-Cheetah
Copy link
Author

Personally I'd prefer to wait on the Godot PR as its still being actively developed, its not abandoned nor stalled nor rejected by Godot, it had another commit just last week and it does not appear to be finished.

Yeah I noticed that, thinking I jumped the gun.
Some things are still being worked after compile.
Idk if to post my compiled build of it since I got it.

@@ -29,6 +26,9 @@
/* TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE */
/* SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */
/**************************************************************************/
/* Work done by Octetdev2 and BSChad. */
/* Merging done by Wasabi_Cheetah. */
/**************************************************************************/
Copy link
Member

@decryptedchaos decryptedchaos Oct 31, 2024

Choose a reason for hiding this comment

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

About this, we can see what the rest of the team says.

But for future references, this is generally not a good practice to add credits to the preamble, if we allowed one person to do this everyone would want to do it and then suddenly you have 100 names in the preamble.

Copy link
Member

@Spartan322 Spartan322 Oct 31, 2024

Choose a reason for hiding this comment

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

Co-authors are supposed to be added to the commit message anyway, like what I did with 2d1c65b for WhalesState, but this won't be merged anyway cause the Godot PR is still WIP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants