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

feat(JOML): migrate polyworlds #35

Merged
merged 16 commits into from
Jan 26, 2021
Merged

Conversation

pollend
Copy link
Member

@pollend pollend commented Jan 18, 2021

This is a complete migration of Polyworlds. I moved in line2f and Poly2f in from termath and we can work out if we want to move them to joml-ext. not sure how it would fit in with the rest of the stuff though. we would basically be adding a winged edge data structure to joml-ext. not sure how that fits in with the rest of code current in joml-ext.

Depends On

Required By

@pollend pollend force-pushed the feat/joml-migrate-polyworlds branch from 0618307 to a259e2e Compare January 24, 2021 16:58
Copy link
Member

@Cervator Cervator left a comment

Choose a reason for hiding this comment

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

Looks fine to me and tests out. Two remaining items:

  • Line2f and Poly2f should probably be moved to joml-geom particularly if they came from TeraMath anyway
  • There are 3 probably simple unit test failures

Additional discussion on a final tiny use of stuff from CommonWorld happened after I did my review and testing

Cervator pushed a commit to Terasology/CommonWorld that referenced this pull request Jan 24, 2021
@jdrueckert
Copy link
Member

* `Line2f ` and `Poly2f` should probably be moved to `joml-geom` particularly if they came from TeraMath anyway

@Cervator These seem to me rather PolyWorld specific. They are part of the delaunay package which includes even more classes used for the triangulation logic, e.g. Vertex, which we afaik don't use widely.
If we were to perceive Line2f and Poly2f as joml primitives (from my point of view they're not), we IMO would need to add a whole bunch more to make the level of structures we see as joml primitives consistent.

@Cervator
Copy link
Member

These seem to me rather PolyWorld specific

That was my thought initially, yup, with PolyWorld just being the library module for poly things related to world gen. Then Michael mentioned they actually came from TeraMath, which threw me off and made me wonder if they were more "generic" and should be in a literal lib rather than a library module. If we don't expect to use them anywhere else then PolyWorld makes sense 👍 Then that just makes me wonder why they were in TeraMath in the first place, but then we probably won't win any awards for being super organized :-)

src/main/java/org/terasology/math/delaunay/Line2f.java Outdated Show resolved Hide resolved
src/main/java/org/terasology/math/delaunay/Line2f.java Outdated Show resolved Hide resolved
src/main/java/org/terasology/math/delaunay/Line2f.java Outdated Show resolved Hide resolved
src/main/java/org/terasology/math/delaunay/Line2f.java Outdated Show resolved Hide resolved
Comment on lines +49 to +50
public static final int SECTOR_SIZE = 1024;
public static final int SECTOR_POWER = Integer.numberOfTrailingZeros(SECTOR_SIZE);
Copy link
Member

Choose a reason for hiding this comment

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

Where do these values come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want the dependency to Sector so the sector area is defined her and allocated as a BlockRegion

https://github.com/Terasology/CommonWorld/blob/93cc9918d28b29d9c64e3a8d884edcbc82c6dccb/src/main/java/org/terasology/commonworld/Sector.java#L33

import org.terasology.joml.geom.Rectanglef;
import org.terasology.math.geom.BaseRect;

public class Line2f {
Copy link
Contributor

Choose a reason for hiding this comment

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

@Cervator I would not throw this into joml-ext right away, but only after careful consideration (let's try to keep that lib high quality instead of dumping things in there).

We already have LineSegmentf which is the same thing in 3D, missing some utility functions.

I'll create a ticket on https://github.com/MovingBlocks/joml-ext to remind us about merging the feature set of this class and the LineSegment class we adopted from JOML, and probably provide 2D variants for it. (👉 MovingBlocks/joml-ext#14)

import java.util.Collection;
import java.util.List;

public class Poly2f {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be worth to move this to joml-geom at some point - ideally, when we know (or can think of) good use cases where this class would be used.

Would definitely put that on the backlog, though.

@skaldarnar skaldarnar requested a review from jdrueckert January 26, 2021 09:46
@skaldarnar skaldarnar self-requested a review January 26, 2021 09:46
@skaldarnar skaldarnar dismissed Cervator’s stale review January 26, 2021 22:18

We'll take care of moving things to joml-ext later, as discussed.

@skaldarnar skaldarnar merged commit 532842d into develop Jan 26, 2021
@skaldarnar skaldarnar deleted the feat/joml-migrate-polyworlds branch January 26, 2021 22:19
skaldarnar pushed a commit to Terasology/Lost that referenced this pull request Jan 26, 2021
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.

4 participants