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

chore: refactor Region to GraphRegion #26

Merged
merged 1 commit into from Oct 14, 2020
Merged

chore: refactor Region to GraphRegion #26

merged 1 commit into from Oct 14, 2020

Conversation

Wdestroier
Copy link
Contributor

@Wdestroier Wdestroier commented Oct 2, 2020

Hi! I solved the name clash and changed the Region class name to GraphRegion as suggested.

Note: On merging this, Lost's references to PolyWorld::Region will break and need to be adjusted to GraphRegion.

@jdrueckert jdrueckert changed the title Issue #24 chore: refactor Region to GraphRegion Oct 2, 2020
Copy link
Member

@jdrueckert jdrueckert left a comment

Choose a reason for hiding this comment

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

Hi @Wdestroier,
thanks for the PR.
I believe you missed a few spots though. For instance, LevelSpawnSystem still imports org.terasology.polyworld.graph.Region which, expectedly, it cannot find anymore now.
Please adjust these spots as well 🙂

@Wdestroier
Copy link
Contributor Author

Hi! @jdrueckert I found the LevelSpawnSystem class in the org.terasology.lost package, am I missing any dependency?

@jdrueckert
Copy link
Member

jdrueckert commented Oct 3, 2020

@Wdestroier Yes you're absolutely right, sorry about that, my brain must be a bit gooey already 🙄
I just wanted to test your PR and saw that it doesn't compile. But that's because Lost depends on PolyWorld and uses the Region. So in order to not break the Lost gameplay we'd need to adjust it to the change in here.
Are you up for adding another PR and then we can merge both?

@jdrueckert
Copy link
Member

So we had another potential contributor ask for taking #24 (ping @gaurav9822). I mentioned that they maybe can take on the Lost side of this.

@@ -102,11 +102,11 @@ public Vector3f getSpawnPosition(EntityRef entity) {
GraphFacet graphs = worldRegion.getFacet(GraphFacet.class);
WhittakerBiomeModelFacet model = worldRegion.getFacet(WhittakerBiomeModelFacet.class);
Vector2f pos2d = new Vector2f(pos.getX(), pos.getZ());
CirclePickerClosest<org.terasology.polyworld.graph.Region> picker = new CirclePickerClosest<>(pos2d);
CirclePickerClosest<org.terasology.polyworld.graph.GraphRegion> picker = new CirclePickerClosest<>(pos2d);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should now be possible without the fully qualified name (as there is no name clash anymore)

Suggested change
CirclePickerClosest<org.terasology.polyworld.graph.GraphRegion> picker = new CirclePickerClosest<>(pos2d);
CirclePickerClosest<GraphRegion> picker = new CirclePickerClosest<>(pos2d);

This change would likely need an import, though.


for (Graph g : graphs.getAllGraphs()) {
BiomeModel biomeModel = model.get(g);
for (org.terasology.polyworld.graph.Region r : g.getRegions()) {
for (org.terasology.polyworld.graph.GraphRegion r : g.getRegions()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (org.terasology.polyworld.graph.GraphRegion r : g.getRegions()) {
for (GraphRegion r : g.getRegions()) {

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.

None yet

3 participants