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
Initial SolarSystemGenerator design #597
Initial SolarSystemGenerator design #597
Conversation
This pull request introduces 1 alert when merging 5001c5e into 9bfdca7 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging fa5bad0 into 9bfdca7 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm a modder and I want to change one element, how much will I have to re-write the same code, but with a small tweak? For example, what if I want to make a system with a black hole instead of a sun - will I have to rewrite the whole SolarSystemGenerator
to change the radius calculation?
this.radius = radius; | ||
} | ||
|
||
public float getRadius() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think for this field, it's important to have clear documentation about what this field should represent. The way we discussed it, radius corresponds to the distance from the center to the farthest element in the system. A modder designing a new module might not realize that, though. Imagine someone building a system with only asteroids and no planets, sun, etc - they might think that the radius is zero, which will cause their asteroids to be placed inside another system.
It might be worth having a further discussion about documentation for interfaces and abstract classes. In my opinion, they should have documentation on either the variables or the getters/setters, so that people who want to extend it know what it is and how it works.
/** | ||
* This method initializes the SolarSystemGenerators. How many generators are initialized depends on the number | ||
* of SolarSystems the world is set to have. When there are multiple types of SolarSystemGenerators available, this | ||
* method chooses randomly from the list of all generators to decide which to create. | ||
* | ||
* A Class variable is used in this method to allow new instances to be made of each type of available SolarSystemGenerator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment is necessary, but it'd be worth asking @NicholasBatesNZ or @Cervator.
*/ | ||
public void placeSystems() { | ||
for (SolarSystemGenerator generator : activeSolarSystemGenerators) { | ||
Vector2 position = getSolarSystemPosition(activeSolarSystemGenerators, generator.getRadius()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "get" is a bit misleading here - you're calculating it, not retrieving an existing value
initializeRandomSolarSystemGenerators(); | ||
/** | ||
* This method iterates through the loop of SolarSystemGenerators that have been initialized and assigns each of | ||
* them a position in the world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period
} | ||
|
||
/** | ||
* This method runs a loop which tests 20 random angles at increasing radii starting from the center of the world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: it's 20 angles at a given radius length, right? And then after those 20, it increases again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! This is the game's original logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify that in the documentation?
solarSystemRadius += groundHeight; | ||
solarSystemRadius += Const.ATM_HEIGHT; | ||
} else { | ||
//add the total height a belt takes up in the SolarSystem (groundHeight will already be negative) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it negative?
solarSystemRadius -= groundHeight; | ||
} | ||
//prevents planets from touching in their orbits | ||
solarSystemRadius += Const.PLANET_GAP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be times the number of planets minus one?
} | ||
|
||
/** | ||
* This places mazes within the outer edge of the solar system. It check to make sure they are places at least 15 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"checks", "placed"
|
||
/** | ||
* This places mazes within the outer edge of the solar system. It check to make sure they are places at least 15 | ||
* degrees apart so they do not overlap. Currently, it only works properly for two maze SolarSystem (this is the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"two-maze SolarSystems" ?
* game default) | ||
* TODO: refactor to allow for more mazes | ||
*/ | ||
void placeMazes() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this supposed to be package-level?
… produces its own radius
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two main thoughts:
- I don't think that any code should specifically address the concept of multiple suns - see my comments below.
- If there's a particular order for how things need to get invoked, make sure that it always happens that way (again, see my comments below).
This is very exciting progress!
@@ -51,11 +58,10 @@ public WorldBuilder(Context context, int numSystems) { | |||
* of SolarSystemGenerators. | |||
*/ | |||
private void populateSolarSystemGeneratorList() { | |||
|
|||
for (Class generator : context.get(ModuleManager.class).getEnvironment().getSubtypesOf(SolarSystemGenerator.class)) { | |||
try { | |||
SolarSystemGenerator solarSystemGenerator = (SolarSystemGenerator) generator.newInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you'll be using the Class to make a new instance, why not store the Class, rather than an instance that isn't getting used?
for (SolarSystemGenerator generator : activeSolarSystemGenerators) { | ||
Vector2 position = calculateSolarSystemPosition(activeSolarSystemGenerators, generator.getRadius()); | ||
generator.setPosition(position); | ||
//Printout of generator position for for testing (as these positions don't have a representation in the game yet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)
} | ||
|
||
/** | ||
* This method runs a loop which tests 20 random angles at increasing radii starting from the center of the world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify that in the documentation?
* This method iterates through the loop of SolarSystemGenerators that have been initialized and assigns each of | ||
* them a position in the world. | ||
*/ | ||
public void positionSolarSystems() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this public? Is this going to be called by other classes?
/** | ||
* This method initiates the build process of each SolarSystemGenerator instance. | ||
*/ | ||
private void buildSolarSystems() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is happening before the positioning, it should be earlier in the file (for readability)
} | ||
|
||
/** | ||
* This method is used to choose a valid position to insert a belt into. It chooses a position that is between two planets | ||
* (not on the edge of the system and not previously used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
)
float distanceFromSun = featureGenerator.getPosition().dst(this.getPosition()); | ||
distanceFromSun += Const.PLANET_GAP; | ||
distanceFromSun += generator.getRadius(); | ||
distanceFromSun += generator.getRadius(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a getDiameter function to FeatureGenerator? While it's not strictly necessary, it'd be nicer to read ;)
setPlanetCount(5); | ||
setPossibleBeltCount(1); | ||
setMazeCount(2); | ||
setSunCount(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These four lines seem to me like they should really be one method with four parameters, possibly in the abstract class's constructor
initializeRandomSunGenerators(); | ||
initializeRandomPlanetGenerators(); | ||
initializeRandomMazeGenerators(); | ||
initializeRandomBeltGenerators(.8f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do not seem like they require a specific order, even though they do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These actually do not have a specific order. Each one just get random Planet, Maze, Belt, or Sun types and adds instances of them to the activeFeatureGenerators list
calculateMazePositions(); | ||
calculatePlanetPositions(); | ||
calculateBeltPositions(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way that this could be partially or fully moved into the abstract class? Modders will have to copy-paste several lines of code for the basic build process to happen
…tor names, refactor configs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's part one of my review - stopping for tonight
public class BeltConfig { | ||
|
||
public final String name; | ||
public final ArrayList<ShipConfig> tempEnemies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these?
public final String name; | ||
public final ArrayList<ShipConfig> tempEnemies; | ||
public final SpaceEnvConfig envConfig; | ||
public final ArrayList<ShipConfig> constEnemies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these?
public final ArrayList<ShipConfig> tempEnemies; | ||
public final SpaceEnvConfig envConfig; | ||
public final ArrayList<ShipConfig> constEnemies; | ||
public final ArrayList<ShipConfig> constAllies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these?
public final ArrayList<ShipConfig> constEnemies; | ||
public final ArrayList<ShipConfig> constAllies; | ||
public final TradeConfig tradeConfig; | ||
public final ArrayList<ShipConfig> innerTempEnemies; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these? I'm guessing these fields were copy-pasted from the old code. Can you figure out what they represent and name them accordingly, so that others don't have to rummage through the rest of the codebase to figure out what they are? ;)
public final ArrayList<ShipConfig> constAllies; | ||
public final TradeConfig tradeConfig; | ||
public final ArrayList<ShipConfig> innerTempEnemies; | ||
public final boolean hard; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "isHard" might be a better name? "hard" does not sound like specifically a boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately it will be hard to change the name, as it needs to be named the same as the key in the JSON will be named
configsToLoad.put(name, beltConfig); | ||
} | ||
|
||
Set<ResourceUrn> configUrnList = Assets.getAssetHelper().listAssets(Json.class, "asteroidBeltsConfig", new Name("engine")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above about generification
Set<ResourceUrn> configUrnList = Assets.getAssetHelper().listAssets(Json.class, "asteroidBeltsConfig", new Name("engine")); | ||
|
||
for (ResourceUrn configUrn : configUrnList) { | ||
rootNode = Validator.getValidatedJSON(configUrn.toString(), "engine:schemaSystemsConfig"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
|
||
BeltConfig config = configsToLoad.get(name); | ||
|
||
// TODO : Maybe add sanity checks for config? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
// TODO : Maybe add sanity checks for config? | ||
|
||
config.tempEnemies.addAll(ShipConfig.loadList(node.has("temporaryEnemies") ? node.getJSONArray("temporaryEnemies") : null, hullConfigs, itemManager)); | ||
config.innerTempEnemies.addAll(ShipConfig.loadList(node.has("innerTemporaryEnemies") ? node.getJSONArray("innerTemporaryEnemies") : null, hullConfigs, itemManager)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these do?
public final ArrayList<ShipConfig> innerTempEnemies; | ||
public final boolean hard; | ||
|
||
public SolarSystemConfig(String name, ArrayList<ShipConfig> tempEnemies, SpaceEnvConfig envConfig, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the Builder pattern might be better here, to avoid having a bunch of parameters listed like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IsaacLic Turns out most of the parameters are pretty useless, as the are only used to pass in empty arrays or objects, and then those are set from JSON data by the load() method. I'm changing it so that those empty objects are initialized in the constructor instead of being passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly minor comments
//These ArrayLists hold instances of any class which extends SolarSystemGenerator or FeatureGenerator, respectively | ||
private ArrayList<SolarSystemGenerator> solarSystemGenerators = new ArrayList<>(); | ||
private ArrayList<FeatureGenerator> featureGenerators = new ArrayList<>(); | ||
//These ArrayLists hold an instance of any class which extends SolarSystemGenerator or FeatureGenerator, respectively |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, I think it's not an instance if it's the actual Class
} | ||
|
||
private void load(HullConfigManager hullConfigs, boolean b, ItemManager itemManager) { | ||
JSONObject rootNode = Validator.getValidatedJSON("engine:systemsConfig", "engine:schemaSystemsConfig"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See BeltConfigManager
about my JSON comments
configsToLoad.put(name, solarSystemConfig); | ||
} | ||
|
||
Set<ResourceUrn> configUrnList = Assets.getAssetHelper().listAssets(Json.class, "systemsConfig", new Name("engine")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be made module-friendly
* This class represents one 'ring' around a SolarSystem's center. It is used to keep track of what Features a SolarSystemGenerator | ||
* should generate. It keeps track of where those Features will be placed, and whether or not a Feature has been chosen | ||
* for a specific orbital. Each orbital has a width, which is the distance from the inner edge of the orbital to the | ||
* outer edge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent javadoc
public class Orbital { | ||
int positionInSolarSystem; | ||
float width; | ||
float startingDistanceFromSystemCenter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the difference between this and positionInSolarSystem
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IsaacLic positionInSolarSystem represents the cardinal order of this orbital. Like the orbital closes to the sun is orbital 0, then orbital 1, etc. I'll come up with a better name
distanceFromSolarSystemCenter += featureGenerator.getRadius(); | ||
((BeltGenerator) featureGenerator).setDistanceFromCenter(distanceFromSolarSystemCenter); | ||
//remove the planet that was going to be placed in this orbital | ||
featureGeneratorsToRemove.add(solarSystemOrbitals.get(orbitInsertionPosition).getFeatureGenerator()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not clear on what this does
* It will only choose FeatureGenerators which are subclasses of {@link SunGenerator}. | ||
* It will continue to make new instances until the correct number of Suns is reached | ||
* It will make one sun generator instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this line of comment is likely unnecessary
e.printStackTrace(); | ||
} | ||
protected void initializeRandomSunGenerator() { | ||
int index = SolRandom.seededRandomInt(featureGeneratorTypes.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do still need a while loop to ensure that the sun actually gets created
@@ -204,11 +275,12 @@ protected void initializeRandomPlanetGenerators() { | |||
* It will continue to make new instances until the correct number of Mazes is reached | |||
*/ | |||
protected void initializeRandomMazeGenerators() { | |||
int mazesLeft = getMazeCount(); | |||
//we will initialize up to 12 mazes to ensure they fit around the SolarSystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking
* @param secondAngleToCheck angle to compare it to | ||
* @param degrees how many degrees apart to ensure angles are between | ||
* @param degrees how many degrees apart to ensure angles are between |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably too much space ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My brain got fried long before I finished this review. Remind me to merge more often...
//World Generation will be initiated from here | ||
worldBuilder.buildWithRandomSolarSystemGenerators(); | ||
//this is just temporarily loaded here so that current world-gen implementation works |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use //TODO: comment
so that it gets highlighted for future action
} catch (InstantiationException | IllegalAccessException e) { | ||
e.printStackTrace(); | ||
} | ||
solarSystemGeneratorTypes.add(generator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private ModuleManager moduleManager = context.get(ModuleManager.class);
...
solarSystemGeneratorTypes.addAll(moduleManager.getEnvironment().getSubtypesOf(SolarSystemGenerator.class));
} | ||
} | ||
} | ||
|
||
public void buildWithRandomSolarSystemGenerators() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No javadocs? 😢
SolarSystemGenerator s = solarSystemGenerator.newInstance(); | ||
s.setFeatureGeneratorTypes(featureGeneratorTypes); | ||
s.setSolarSystemConfigManager(solarSystemConfigManager); | ||
s.setSolarSystemNumber(i); | ||
generatorArrayList.add(s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that this variable doesn't live very long but please don't get into the habit of single letter names. Something like generator
would be better
Vector2 position = calculateSolarSystemPosition(activeSolarSystemGenerators, generator.getRadius()); | ||
generator.setPosition(position); | ||
//Printout of generator position for testing (as these positions don't have a representation in the game yet) | ||
System.out.println(generator + " position: " + generator.getPosition().x + ", " + generator.getPosition().y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be using a logger rather than stdio.
private static final Logger logger = LoggerFactory.getLogger(WorldBuilder.class);
... // any of:
logger.info("msg");
logger.debug("msg");
logger.error("msg");
private void calculatePlanetPositions() { | ||
int orbitalPosition = 0; | ||
for (FeatureGenerator featureGenerator : activeFeatureGenerators) { | ||
Vector2 result = new Vector2(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vector pool
result.add(position); | ||
generator.setPosition(result); | ||
|
||
System.out.println(generator + " distance from center: " + generator.distanceFromSolarSystemCenter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger
is placed, a planet is removed and a maze set in its place */ | ||
featureGeneratorsToRemove.add(solarSystemOrbitals.get(orbitInsertionPosition).getFeatureGenerator()); | ||
solarSystemOrbitals.get(orbitInsertionPosition).setFeatureGenerator(featureGenerator); | ||
System.out.println("Distance for belt: " + distanceFromSolarSystemCenter); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger
} | ||
|
||
/** | ||
* This method checks if two angles are within a certain number of degrees of each other or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it only checks two angles then why is the first argument a list?
return radius; | ||
} | ||
|
||
public void setPositioned(boolean p) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
single letter var name
Description
This PR defines some initial sections of the SolarSystemGenerator class. This class is an abstract class that will handle much of the logic behind SolarSystem generation. So far, this code creates placements for SolarSystems and Mazes, and generates the correct number of SolarSystemGenerators, MazeGenerators, and PlanetGenerators.
Testing
When you run the game, you will see print statements describing what position has been set for each generator created (these do not show up in the actual game yet).
Design Questions
I'm not sure about my use of Class variable to instantiate new Maze and Planet generators in the SolarSystemGenerator class. Previously, the code just called featureGenerator.build() on each available FeatureGenerator. The issue with this is that each FeatureGenerator actually needs to be initialized as it's own object, instead of being a reference to the one is the list (So that they can each have their own position, size, etc.). However, I'm unsure if this is the best way to handle that.
Another issue is the order of world generation. Currently, WorldBuilder places each SolarSystemGenerator before calling the build method of each system. This means that the radius of the System has to be determined before build() is called. Therefore, the value of how many planets the SolarSystemGenerator will create needs to be known before any methods from SolarSystemGeneratorImpl can be called. Therefore, the only way for SolarSystemGeneratorImplementations to set the number of planets before their build() method is called is to pass it into the super() constructor. That way it can be taken into account when the superclass's constructor builds the system radius.