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

Simple World Generator and Base / Flag Generation #64

Merged
merged 16 commits into from May 23, 2018

Conversation

4 participants
@dacharya64
Copy link
Contributor

dacharya64 commented May 19, 2018

This adds in a new world generator (Light and Shadow (Simple)) that is just a plane with bases that spawn 30 units from the center of the world (at x=-30, and x=30). The flag also spawns at the center of the base.

Test by loading up the L&S module and choosing Light and Shadow (Simple) as world generator.

@Cervator Cervator requested review from skaldarnar and nihal111 May 20, 2018

@Cervator Cervator added this to Backlog in GSOC 2018 - Light and Shadow via automation May 20, 2018

@Cervator Cervator moved this from Backlog to In Progress in GSOC 2018 - Light and Shadow May 20, 2018

@nihal111

This comment has been minimized.

Copy link
Member

nihal111 commented May 20, 2018

Started testing this out with a new environnment, for anyone else testing this out-

  1. City and LightAndShadow are both at develop. Make sure to fetch the modules and switch branches.
  2. Auto fetch dependencies by groovyw didn't work well for Pathfinding and NameGenerator, needed to fetch those modules manually. Likely needs a new release?
@nihal111
Copy link
Member

nihal111 left a comment

Tested it locally.
I end up getting only a 5x4 base for the Red flag (x=+30)
and a 5x5 base for the Black flag (x=-30).
Both should have been 5x9 bases as per the region definition by minmax.
My guess is that the base is cut across multiple chunks and the logic inside generateChunk method as defined in the BaseRasterizer runs only for the two chunks which encompass the two centerBasePositions.

Solution off the top of my head is to define a BaseProvider to set Base for all entirety of the base and a new Flag at the center. The BaseRasterizer would simply create a stone block for all Base.

Very likely, a cleaner elegant solution exists.


Vector3i centerBasePosition = new Vector3i(entry.getKey());
int extent = entry.getValue().getExtent();
centerBasePosition.add(0, extent, 0);

This comment has been minimized.

@nihal111

nihal111 May 20, 2018

Member

Why add extent to y only to subtract it in the next line?

Vector3i centerBasePosition = new Vector3i(entry.getKey());
int extent = entry.getValue().getExtent();
centerBasePosition.add(0, extent, 0);
Vector3i min = new Vector3i(centerBasePosition.x() - extent + 2, centerBasePosition.y() - extent, centerBasePosition.z() - extent);

This comment has been minimized.

@nihal111

nihal111 May 20, 2018

Member

Why do centerBasePosition.x() - extent + 2? Shouldn't it be symmetric to z?
Do you have a rectangular base in mind? What is 2?

}
//place flags
if (centerBasePosition.x > 0){
if (chunkRegion.getRegion().encompasses(centerBasePosition)) {

This comment has been minimized.

@nihal111

nihal111 May 20, 2018

Member

Create a Vector3i flagPosition with centerBasePosition.x(), centerBasePosition.y() - extent + 1, centerBasePosition.z() and check for the chunkRegion to encompass flagPosition

@dacharya64

This comment has been minimized.

Copy link
Contributor

dacharya64 commented May 20, 2018

Thanks for the clarification here! I was a bit confused as to how the generation works here. I'll take another look at this.

@skaldarnar
Copy link
Contributor

skaldarnar left a comment

Tested it locally, too. Nothing to add to @nihal111 review (for now).
I think that facets and borders are the current way to solve the problem of cut-off features like the bases.

dacharya64 added some commits May 20, 2018

Merge pull request #1 from dacharya64/flags
Flag component that adds flag to inventory on activate
@dacharya64

This comment has been minimized.

Copy link
Contributor

dacharya64 commented May 20, 2018

Added code for flag to be able to be picked up onActivate (pressing E). Test with Terasology/LightAndShadowResources#23. Tried to do this as a separate PR but it got complicated.

dacharya64 added some commits May 20, 2018

Removes commented out inventory code and changes flag logic
Changes flag logic to allow for possibly more types of flags.
Updates base spawning info so no longer cut by rasterizer chunking
Base size and creation is now handled by BaseProvider, with BaseRasterizer just placing appropriate blocks
@dacharya64

This comment has been minimized.

Copy link
Contributor

dacharya64 commented May 21, 2018

Fixed issues with the rendering of the base blocks (moving some of that logic to Provider / Facet) being cut off so now the base should spawn the appropriate size.

@nihal111
Copy link
Member

nihal111 left a comment

Neat! Looks great. Tests out perfectly too. :)
A few nitpicks, left inline comments.

I think we should add the FlagComponent. It'd be handy later and would be a cleaner, more natural way to go about the destroy check than matching prefab.

@Requires(@Facet(SurfaceHeightFacet.class))

public class BaseProvider implements FacetProvider {
int baseExtent = 2; //determines size of base (# of tiles from center)

This comment has been minimized.

@nihal111

nihal111 May 21, 2018

Member

Can we change that isolated comment into javadoc? :)
Also probably mention "Base is a square of side 2*baseExtent + 1 with the flag at the center"

This comment has been minimized.

@skaldarnar

skaldarnar May 21, 2018

Contributor

I guess this is all WIP, right? Otherwise, define the BASE_EXTEND as constant (similarly for the base center positions)

This comment has been minimized.

@dacharya64

dacharya64 May 22, 2018

Contributor

What do I need to do to turn things into javadoc? I saw this (https://github.com/MovingBlocks/Terasology/wiki/JavaDoc) but wasn't sure about it.

This comment has been minimized.

@Cervator

Cervator May 22, 2018

Member

Easy! Literally write it /** Like this */ instead of // Like this

Usually I like the one liner for field variables that can be described really easily. For longer stuff or methods use the multiline version:

/**
 * Stuff.
 */

@Override
public void process(GeneratingRegion region) {
Border3D border = region.getBorderForFacet(BaseFacet.class).extendBy(0, 8, 4);

This comment has been minimized.

@nihal111

nihal111 May 21, 2018

Member

Why do we need the extension? I removed and tested it with abnormally large bases and it seems to work.


EntityRef flagTaker = event.getInstigator();
if (blockComponent.getBlock().getBlockFamily().getURI().toString().equals("LightAndShadowResources:blackFlag")) {
flagTaker.send(new GiveItemAction(flagTaker, blockFactory.newInstance(blockManager.getBlockFamily("LightAndShadowResources:blackFlag"))));

This comment has been minimized.

@nihal111

nihal111 May 21, 2018

Member

GiveItemAction is deprecated. Shift to InventoryManager's giveItem method instead.

@Requires(@Facet(SurfaceHeightFacet.class))

public class BaseProvider implements FacetProvider {
int baseExtent = 2; //determines size of base (# of tiles from center)

This comment has been minimized.

@skaldarnar

skaldarnar May 21, 2018

Contributor

I guess this is all WIP, right? Otherwise, define the BASE_EXTEND as constant (similarly for the base center positions)

int baseExtent = 2; //determines size of base (# of tiles from center)

Vector3i centerRedBasePosition = new Vector3i(30, 10, 0);
Region3i redBaseRegion = Region3i.createFromMinMax(new Vector3i(centerRedBasePosition.x() - baseExtent, centerRedBasePosition.y(), centerRedBasePosition.z() - baseExtent), new Vector3i(centerRedBasePosition.x() + baseExtent, centerRedBasePosition.y(), centerRedBasePosition.z() + baseExtent));

This comment has been minimized.

@skaldarnar

skaldarnar May 21, 2018

Contributor

To me it looks like the code determining the base region could be refactored to a method and applied to both center positions...

@nihal111
Copy link
Member

nihal111 left a comment

Few nitpicks-
Use private whenever access is only from with class.
Java docs should go on top of the variable definition.
You don't need to extend with 0,0,0

@@ -46,13 +47,19 @@ public void setSeed(long seed) {

@Override
public void process(GeneratingRegion region) {
Border3D border = region.getBorderForFacet(BaseFacet.class).extendBy(0, 8, 4);
Border3D border = region.getBorderForFacet(BaseFacet.class).extendBy(0, 0, 0);

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

This is unnecessary. Remove the extension altogether.
Border3D border = region.getBorderForFacet(BaseFacet.class);

This comment has been minimized.

@dacharya64

dacharya64 May 22, 2018

Contributor

I wasn't sure how to remove this since the BaseFacet (as well as the SparseObjectFacet3D which it extends) takes a border as part of the constructor. Should I create a new SparseObjectFacet3D constructor that doesn't need a border as a param?

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

I don't think you understand.
Remove the extension == Remove the .extendBy(x,y,z)
Keep Border3D border = region.getBorderForFacet(BaseFacet.class);
in place of the present Border3D border = region.getBorderForFacet(BaseFacet.class).extendBy(0, 0, 0);

This comment has been minimized.

@dacharya64

dacharya64 May 22, 2018

Contributor

Ah ok, got it!

@@ -26,15 +26,16 @@
@Requires(@Facet(SurfaceHeightFacet.class))

public class BaseProvider implements FacetProvider {
int baseExtent = 2; //determines size of base (# of tiles from center)
/** Base is a square of side 2 * BASE_EXTENT + 1 with the flag at the center */
static final int BASE_EXTENT = 2; /** determines size of base (# of tiles from center) */

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

Keep a multi-line java doc for this.

/**
 * Determines size of base (# of tiles from center)
 * Base is a square of side 2 * BASE_EXTENT + 1 with the flag at the center
 */
private static final int BASE_EXTENT = 2; 
int baseExtent = 2; //determines size of base (# of tiles from center)
/** Base is a square of side 2 * BASE_EXTENT + 1 with the flag at the center */
static final int BASE_EXTENT = 2; /** determines size of base (# of tiles from center) */
static final Vector3i CENTER_RED_BASE_POSITION = new Vector3i(30, 10, 0); /** position of red base */

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

Keep a single line java doc for these.

/** Position of Red base */
private static final Vector3i CENTER_RED_BASE_POSITION = new Vector3i(30, 10, 0);

likewise for black

@@ -44,33 +44,11 @@
@In
private BlockManager blockManager;

@ReceiveEvent
@ReceiveEvent //give player inventory items on game start

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

Switch to java doc.

@nihal111
Copy link
Member

nihal111 left a comment

Almost there!

inventoryManager.giveItem(flagTaker, EntityRef.NULL, blockFactory.newInstance(blockManager.getBlockFamily("LightAndShadowResources:blackFlag")));
}
if (blockComponent.getBlock().getBlockFamily().getURI().toString().equals("LightAndShadowResources:redFlag")) {
if (teamComponent.team.equals("red")){

This comment has been minimized.

@nihal111

nihal111 May 22, 2018

Member

missing space between ) and {
Please do a code cleanup (Ctrl + Alt + L on IntelliJ)
You can also reformat code on a whole directory. Open up the project view with (Alt + 1) and right click a folder + click "Reformat Code"

@dacharya64

This comment has been minimized.

Copy link
Contributor

dacharya64 commented May 22, 2018

Just pushed code cleanup changes!

@nihal111
Copy link
Member

nihal111 left a comment

All wildcard imports should be replaced with separate imports. Try setting up the Terasology checkstyle for IntelliJ, I think it's not enabled.
https://github.com/MovingBlocks/Terasology/wiki/Checkstyle

import org.terasology.cities.DefaultBlockType;
import org.terasology.cities.SimpleBiomeProvider;
import org.terasology.cities.bldg.Building;
import org.terasology.cities.*;

This comment has been minimized.

@nihal111

nihal111 May 23, 2018

Member

Please set the limit to 100 for imports to use wildcard. I think we have this in the Terasology checkstyle.

@skaldarnar
Copy link
Contributor

skaldarnar left a comment

Approving this as there's not much left to be done not mentioned by already.
@nihal111 feel free to merge whenever this seems ready to you, I most likely won't have a look at it again before the meeting tomorrow.

import org.terasology.world.generation.World;
import org.terasology.world.generation.WorldBuilder;
import org.terasology.world.generator.RegisterWorldGenerator;
import org.terasology.world.generator.plugin.WorldGeneratorPluginLibrary;

import org.terasology.las.LaSFloraRasterizer;
//import org.terasology.core.world.generator.facetProviders.EnsureSpawnableChunkZeroProvider;

This comment has been minimized.

@skaldarnar

skaldarnar May 23, 2018

Contributor

don't commit commented code - it should either be in the history anyways (and thus can be restored) or is not needed at all (then why commit it).

@nihal111

This comment has been minimized.

Copy link
Member

nihal111 commented May 23, 2018

Merging. Good work @dacharya64 :)

@nihal111 nihal111 merged commit bffbefa into Terasology:develop May 23, 2018

GSOC 2018 - Light and Shadow automation moved this from In Progress to Completed May 23, 2018

@dacharya64 dacharya64 moved this from Completed to Notes and Meta in GSOC 2018 - Light and Shadow Jun 19, 2018

@dacharya64 dacharya64 moved this from Notes and Meta to PRs in GSOC 2018 - Light and Shadow Jun 19, 2018

@dacharya64 dacharya64 moved this from PRs to Completed in GSOC 2018 - Light and Shadow Jul 3, 2018

@dacharya64 dacharya64 moved this from Completed to PRs in GSOC 2018 - Light and Shadow Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment