feat(JOML): migrate polyworlds #35
Conversation
pollend
force-pushed
the
feat/joml-migrate-polyworlds
branch
from
0618307
to
a259e2e
Compare
There was a problem hiding this comment.
Looks fine to me and tests out. Two remaining items:
Line2fandPoly2fshould probably be moved tojoml-geomparticularly 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 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. |
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/test/java/org/terasology/polyworld/sampling/PoissonDiscTestView.java
Outdated
Show resolved
Hide resolved
src/test/java/org/terasology/polyworld/sampling/PoissonDiscTestView.java
Outdated
Show resolved
Hide resolved
| public static final int SECTOR_SIZE = 1024; | ||
| public static final int SECTOR_POWER = Integer.numberOfTrailingZeros(SECTOR_SIZE); |
There was a problem hiding this comment.
Where do these values come from?
There was a problem hiding this comment.
I didn't want the dependency to Sector so the sector area is defined her and allocated as a BlockRegion
| import org.terasology.joml.geom.Rectanglef; | ||
| import org.terasology.math.geom.BaseRect; | ||
|
|
||
| public class Line2f { |
There was a problem hiding this comment.
@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 { |
There was a problem hiding this comment.
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.
…tView.java Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
…tView.java Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
Co-authored-by: jdrueckert <jd.rueckert@googlemail.com>
skaldarnar
dismissed
Cervator’s stale review
We'll take care of moving things to joml-ext later, as discussed.
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