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

Skeleton classes for world-gen project #596

Merged
merged 6 commits into from
Jun 22, 2021

Conversation

IsaiahBlanks
Copy link
Contributor

Description

This PR contains classes that represent the skeleton of the world-gen project

Testing

There should be a couple of print statements showing that the Generators are called. The Generator Implementation classes should have their build() method called

Outstanding Work

Several aspects of this PR may need to be changed in the future, including the data structure used to hold the Generators and the particular way those Generators are instantiated

@NicholasBatesNZ
Copy link
Member

Congratulations on your first official PR for GSoC 🎉
You're off to a good start but I can see some problems. The way you've written this doesn't seem to allow for the generic feature generator classes we talked about a few weeks ago, unless I'm missing something and you can enlighten me?
As I see it, the WorldBuilder should only be pulling in two types of classes. 1) classes that extend SolSystemGenerator and 2) classes that extend, let's say, a FeatureGenerator.
You should be thinking in terms of "how modular can I be?" Having the engine explicitly look for a PlanetGenerator is not particularly modular.
Ping me on Discord for more clarity, or to tell me why I'm wrong :)
What is the purpose of the WorldGenerator annotation? It seems a bit superfluous to me.

Copy link
Contributor

@IsaacLic IsaacLic left a comment

Choose a reason for hiding this comment

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

Most of my comments are minor. I have two main comments:

  1. As @NicholasBatesNZ said, I'm not sure what purpose the WorldGenerator annotation is serving. A bit of Javadoc would go a long way, which brings me to my second point:
  2. Your code could definitely benefit from some documentation. Imagine someone encountering your code two years down the line. A bit of documentation could make the difference between five minutes and five hours spent understanding what a class does. IIRC, the policy is to have class-level documentation for every class, and to use method-level documentation whenever necessary. I could be wrong about that, though.

Copy link
Contributor

@IsaacLic IsaacLic left a comment

Choose a reason for hiding this comment

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

Looking much better :)

I have a bunch of minor comments about improving the Javadoc. In short: think about what it'd be like if you were encountering this code for the first time, and had no idea what it did or how it worked. The Javadoc should provide an overview, so that the code can be easily understood.

}

/**
* This method is intended to first set up the SolSystem during world generation and then initialize all the
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand. Does this build the actual SolSystem? If it does, wouldn't the FeatureGenerators have been initialized beforehand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IsaacLic The FeatureGenerators are the next step in the world-gen process. Meaning first the SolSystem's own features are generated (belts, position in the world, deciding which Features to use), then the FeatureGenerators themselves are built (i.e. Planets, Mazes, etc. have their own build() function, and SolSystemGenerator will finish up by calling those). I think the chart in the proposal clarifies a little bit here

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. How about "set up the broader SolSystem" and "use FeatureGenerators to do X" ? That might clarify the relationship between the two

package org.destinationsol.world.generators;

/**
* This class is a concrete implementation of a SolSystem and handles its creation. This class has access to the
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

Copy link
Contributor

@IsaacLic IsaacLic left a comment

Choose a reason for hiding this comment

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

This looks pretty good! I have some minor comments, but nothing that'd be a big refactoring or anything

* This method calls setFeatureGenerators() on each SolSystemGenerator to give each SolSystem access to other
* FeatureGenerators (Planets, Mazes, etc.) so they can be initialized and incorporated into the SolSystem.
*/
private void initializeSolSystemGenerators() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the discussion in Discord about random vs selected generators is worthwhile, but shouldn't hold back the basic worldgen structure from progressing. If you rename this something like initializeSolarSystemGeneratorsUsingRandomGenerators() and rename build() to buildUsingRandomGenerators(), that leaves open the possibility of using selected generators without needing to code a specific implementation of that idea. Lmk if that makes sense to you

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IsaacLic I think the change you are suggesting makes sense (though I might think initializeRandomSolarSystemGenerators() might by a little less wordy. What do you think?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me :)

package org.destinationsol.world.generators;

/**
* This class represents the abstract version of any feature which will populate the game's SolSystems, such as Planets,
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the subclasses create the planets/mazes, or will they be the planets/mazes? It could be that I'm not understanding how you're using the word "feature", but it sounds like the subclasses will be instances of features, meaning that the subclasses would be Planet instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@IsaacLic Yeah I think the word 'be' is more correct. I had meant create as in place themselves into the world. Meaning each SolarSystemGenerator will pick whichever FeatureGenerators it wants to use, and then those FeatureGenerators will be capable of placing an instance of themselves into the game

Edit: After our discussion on Discord, the Generator classes will be responsible for building their respective Classes. Those will be represented in the game during runtime using the classes already in place.

@NicholasBatesNZ NicholasBatesNZ changed the base branch from develop to gsoc2021 June 19, 2021 22:56
import org.destinationsol.world.generators.SolarSystemGenerator;

import java.lang.reflect.Modifier;
import java.util.*;
Copy link
Member

Choose a reason for hiding this comment

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

public abstract void build();

public void setFeatureGenerators(ArrayList<FeatureGenerator> generators) {
featureGenerators = (ArrayList<FeatureGenerator>) generators.clone();
Copy link
Member

Choose a reason for hiding this comment

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

Two questions:

  1. Do we really need the cast here?
  2. Is it necessary to take a deep copy or could we instead use the references with featureGenerators.addAll(generators)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe arraylist.clone() actually makes a shallow copy, so either could work


@Override
public void build() {
for(FeatureGenerator generator : featureGenerators) {
Copy link
Member

Choose a reason for hiding this comment

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

for( -> for ( for consistency
(could also use a .forEach lambda but that's not important :P )

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.*;
Copy link
Member

Choose a reason for hiding this comment

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

star import

package org.destinationsol.world.generators;

/**
* This class is a concrete implementation of a SolarSystem and handles creation of elements
Copy link
Contributor

Choose a reason for hiding this comment

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

SolarSystemGenerator ?

/**
* This class is the starting point for world generation. When a new world is created, this class first retrieves
* all Generator classes and then initiates each SolarSystem's build process. Two types of world generation classes are
* retrieved: those that subclass SolarSystemGenerator and those at subclass FeatureGenerator.
Copy link
Contributor

@IsaacLic IsaacLic Jun 21, 2021

Choose a reason for hiding this comment

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

those that subclass FeatureGenerator

@NicholasBatesNZ NicholasBatesNZ merged commit 9bfdca7 into MovingBlocks:gsoc2021 Jun 22, 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

4 participants