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

Initial PlanetGenerator Setup #602

Merged
merged 5 commits into from
Jul 30, 2021

Conversation

IsaiahBlanks
Copy link
Contributor

Description

This PR adds the default implementation of Planets to the new modular world-gen system. It also sets up the game so that SolarSystems and Planets generated by Generator classes show up in the game.

In this implementation, many options are available to the Planet via getting the config in your particular implementation, and then changing various values

Testing

Run the game, and you should see SolarSystems running with Planets in them, but no belts and no mazes. There are two types of SolarSystems that will generate, the default systems and large SolarSystems. Generate a world with a good amount of systems (at least 4) to make sure you see the different types.

Outstanding Work

Two features still need to be added for Planets. One is the ability to place custom objects/ modify terrain, which will involve modularizing the PlanetObjectsBuilder class to accept types of objects, instead of specific objects. The other is the ability to load custom PlanetConfig files. Additionally, tests need to be written still. There is an issue with testing PlanetGenerators as the require the PlanetConfigs class to be loaded, and this seems to be dependent on the class CollisionMeshLoader, which is throwing an error when running tests

@Cervator
Copy link
Member

Tests out!

Was able to run the game in my Android Studio setup, although I had to delete my local warp module (incompatible with the worldgen changes, although probably an easy fix). Did get some weird errors from trying gradlew clean buildDebug run but that might just be the workspace. gradlew jar run worked fine after I deleted warp

Can confirm the game runs as described, I started with a few more systems than usual and got a nice big game world, no belts or mazes - which is fine even for merging since there's a target topic branch. Was able to travel to the nearest planet and see it looking pretty normal. One oddity: on my way there I flew by a hostile space station - I think those are limited to the second solar system in the classic setup? With worldgen changing it makes sense some things like that will be thrown off, the starter system should probably be special and have fairly mundane spawning rules.

It might also help this PR with a quick description of a test that'd vary a planet or planet type, although that may well be more for a tutorial like Terasology's worldgen one :-)

I did eventually crash, after arriving at a different solar system and heading toward a planet there, with a java.lang.AssertionError: normalizing NaN angle - DS code tends to crash rather overly hard with these assertion errors over even tiny things. We probably have existing bugs floating around still that cause the issue, but maybe this is new since the stacktrace includes some planet gen. Full stacktrace:

com.badlogic.gdx.utils.GdxRuntimeException: java.lang.AssertionError: normalizing NaN angle
	at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.<init>(Lwjgl3Application.java:117)
	at org.destinationsol.desktop.SolDesktop.main(SolDesktop.java:149)
Caused by: java.lang.AssertionError: normalizing NaN angle
	at org.destinationsol.common.SolMath.norm(SolMath.java:107)
	at org.destinationsol.common.SolMath.angle(SolMath.java:354)
	at org.destinationsol.game.planet.PlanetObjectsBuilder.buildGroundShip(PlanetObjectsBuilder.java:336)
	at org.destinationsol.game.planet.PlanetObjectsBuilder.buildGroundEnemies(PlanetObjectsBuilder.java:107)
	at org.destinationsol.game.planet.PlanetObjectsBuilder.createShips(PlanetObjectsBuilder.java:88)
	at org.destinationsol.game.planet.PlanetObjectsBuilder.createPlanetObjs(PlanetObjectsBuilder.java:75)
	at org.destinationsol.game.planet.Planet.update(Planet.java:79)
	at org.destinationsol.game.planet.PlanetManager.update(PlanetManager.java:68)
	at org.destinationsol.game.SolGame.lambda$null$2(SolGame.java:385)
	at java.util.ArrayList.forEach(ArrayList.java:1249)
	at org.destinationsol.game.SolGame.lambda$update$3(SolGame.java:384)
	at java.lang.Iterable.forEach(Iterable.java:75)
	at org.destinationsol.game.SolGame.update(SolGame.java:383)
	at org.destinationsol.SolApplication.update(SolApplication.java:251)
	at org.destinationsol.SolApplication.safeUpdate(SolApplication.java:225)
	at org.destinationsol.SolApplication.render(SolApplication.java:189)
	at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Window.update(Lwjgl3Window.java:392)
	at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.loop(Lwjgl3Application.java:137)
	at com.badlogic.gdx.backends.lwjgl3.Lwjgl3Application.<init>(Lwjgl3Application.java:111)
	... 1 more


If this isn't an obvious bug it may be a call to harden the code somewhere to catch this better and either log more informative details or even skip over something small if it is like a single turret that fails to place (still logging an error, of course

Other stuff:

  • Eventually make a compatibility PR for the warp module and any other modules the GSOC work may break - which I suspect is very few as code in modules so far is really rare
  • Probably just for making an issue to process later, but: are starlanes being placed excessively, to where you can get to many places from many places, rather than have to hop hop hop to get somewhere distant? I got into a way long one to get to an outer system (played with like 6-7 systems). Probably this needs to be balanced by either breaking the starlanes into shorter multi-hop trips or maybe vary the speed based on the length to reach a comparable short-ish duration on any sort of travel.

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.

Egads I have had time to do a semi-thorough review! Probably not really though, it is limited how much focus is available with a newborn half-sleeping behind you. Consider this some starting recommendations rather than necessarily an involved treatise on the architectural applicability of the PR overall - would probably be good with another set of eyes to consider that :-)

I think one thing I run into repeatedly is separating your new/moved code with legacy code. We could probably talk about refactoring naming and such all day long yet that's at least somewhat beyond the scope of your GSOC. So wherever I'm poking at a naming issue feel free to skip if it is preexisting or better yet add some todos or issues so somebody else can return and address it later at some point :-)

@@ -0,0 +1,95 @@
/*
* Copyright 2020 The Terasology Foundation
Copy link
Member

Choose a reason for hiding this comment

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

Happy New Year! Wait, it is almost August? 😁

*/
public class CloudBuilder {
private final List<TextureAtlas.AtlasRegion> cloudTextures = new ArrayList<>();
//Frequency at which clouds will generate
Copy link
Member

Choose a reason for hiding this comment

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

I love seeing variables documented like this, although maybe that's just a weird personal thing. Although I'd use single line javadoc - even if the code is private it highlights nicely in an IDE and I figure it would actually show with javadoc preview even if javadoc generation doesn't exact it. So in other words:

/** Frequency at which clouds will generate */

I'd also leave a newline between each set of var + javadoc. Although again I dunno if this is just a rare personal preference or what not.

* variables, such ship density (how many of the ships are places), are dependent on the specific ShipConfig,
* and so are held within the list of ShipConfigs.
*/
public class OrbitEnemiesBuilder {
Copy link
Member

Choose a reason for hiding this comment

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

OrbitalEnemiesBuilder maybe? Rather than "Orbit" alone

Elsewhere too like the nice javadoc has "Orbit Enemies" which reads funny, "orbital enemies" would be entirely natural :-)

public class OrbitEnemiesBuilder {
private ArrayList<ShipConfig> orbitEnemies = new ArrayList<>();
//Offset Percentage represents how far apart enemies should be made, in terms of circumference of the planet
float offsetPercentage;
Copy link
Member

Choose a reason for hiding this comment

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

Is package scope intentional here? Or meant to be public/private/protected?

calculateSolarSystemPosition(activeSolarSystemGenerators, generator, generator.getRadius());
try {
calculateSolarSystemPosition(activeSolarSystemGenerators, generator, generator.getRadius());
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Any way to narrow this catch a little? Going straight for Exception is a tad wide :-)

I realize this is probably still an improvement on prior state as-is!

}

/**
* This method modifies the percentage of the atmosphere at which low orbit ships start spawning.
Copy link
Member

Choose a reason for hiding this comment

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

The percentage of the atmosphere? Huh. As in, related to height/distance from the surface of a planet? I guess in theory you could think of an atmosphere as thinning out (thus lower percentage the higher you go), but I wonder if a simply height offset or something would be clearer (I could imagine exotic / magical atmospheres that could be equally thick from surface to top...) . As usual probably a separate refactor.

import org.slf4j.LoggerFactory;

/**
* This class is a concrete implementation of a PlanetGenerator and handles its creation. This class creates planets
Copy link
Member

Choose a reason for hiding this comment

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

"its" creation being relevant to the planet generator or a planet itself? Nitpick :-)

@@ -204,12 +208,18 @@ private void calculatePlanetPositions() {
Orbital orbital = solarSystemOrbitals.get(orbitalPosition);
orbital.setFeatureGenerator(generator);

generator.setAngleInSolarSystem(SolRandom.seededRandomFloat(180));
Copy link
Member

Choose a reason for hiding this comment

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

Magic number (descriptive constant for 180 probably would be good?)

* after the build method is called
* @return List of Planets
*/
ArrayList<Planet> getBuiltPlanets() {
Copy link
Member

Choose a reason for hiding this comment

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

"Built" sounds funny to me here. "Generated" ? What exact state are the planets in before, after, and what happens next? Is surface feature placement and other stuff still to do? This is where again a diagram or two would be awesome to show the phases / state changes :-)

@@ -95,7 +104,7 @@ private HullConfigManager setupHullConfigManager(ItemManager itemManager) {

private void setupSolarSystemGenerator() {
solarSystemGenerator.initializeRandomDefaultFeatureGenerators(1f);
solarSystemGenerator.buildFeatureGenerators();
//solarSystemGenerator.buildFeatureGenerators();
Copy link
Member

Choose a reason for hiding this comment

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

Might be good with a quick TODO to note why this is commented out

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.

Good changes! Was able to test a bit more without crashing (but not without dying! Woo)

logger.info("Planet: " + planet.getName() + ", Station Vector: " + distanceToPlanet.toString());
float angle = 0;
//TODO: Determine why distanceToPlanet is occasionally equal to {NaN, NaN}
if (!distanceToPlanet.equals(new Vector2(Float.NaN, Float.NaN))) {
Copy link
Member

Choose a reason for hiding this comment

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

Not really needed right this moment, but it may be worth simply declaring a constant Vector2 somewhere with the NaN values rather than create a new one every check?

* from the center of the world when placing a system. However, in order to ensure a place will be found, the
* number is multiplied by 100
*/
maxWorldRadius = (12 * numberOfSystems * 100);
Copy link
Member

Choose a reason for hiding this comment

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

The comment definitely helps, these values are still magic numbers though :-) Also a low priority but likely replacement with constants would be nice. That would also limit the need for the comment being lengthy if the name of the constants make the code self-documenting (and extra details could go on javadoc on the constants)

@@ -130,7 +138,7 @@ private void positionSolarSystems() {
for (SolarSystemGenerator generator : activeSolarSystemGenerators) {
try {
calculateSolarSystemPosition(activeSolarSystemGenerators, generator, generator.getRadius());
} catch (Exception e) {
} catch (RuntimeException e) {
Copy link
Member

Choose a reason for hiding this comment

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

Better 👍 There are still a lot of RuntimeExceptions that are possible. Not necessarily needed to go more narrow right now, but in addition to printing the stacktrace might it be worthwhile logging a more specific message to this place in the code and its expectations? IIRC there's also a logger method that ends up including the printed stacktrace anyway.

@Cervator Cervator merged commit 01498e3 into MovingBlocks:gsoc2021 Jul 30, 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.

None yet

2 participants