-
Notifications
You must be signed in to change notification settings - Fork 13
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
Generate names for cities #39
Conversation
Names are now assigned to sites on world generation as opposed to on encounter.
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.
Looks fine to me, although I haven't tested yet pending WIP :-)
Would be nice with some quick instructions of what to look for / how to test when ready.
@Override | ||
public void postBegin() { | ||
|
||
settlementEntities = settlementCachingSystem.getSettlementCacheEntity(); | ||
rng = new FastRandom(regionEntityManager.hashCode() & 0x921233); | ||
long seed = regionEntityManager.hashCode() & 0x921233; |
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.
Would it be worth trying to hook into the world seed instead? Also might help to make a variable/constant for the magic string here to explain its meaning (it seems fairly obvious but is a good practice)
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 leaving this one as is is fine.
The names are hooked into the world seed below, this is for the other randoms, like city generation over time. If you hooked it into the seed it wouldn't really matter because all the other factors would be different
src/main/java/org/terasology/dynamicCities/settlements/SettlementEntityManager.java
Show resolved
Hide resolved
src/main/java/org/terasology/dynamicCities/population/CultureComponent.java
Outdated
Show resolved
Hide resolved
src/main/java/org/terasology/dynamicCities/settlements/SettlementEntityManager.java
Outdated
Show resolved
Hide resolved
Updated. Had some questions so I left them as comments :) |
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.
Overall looks good to me. Will test it in game now =)
|
||
@Share(ThemeManager.class) | ||
@RegisterSystem(RegisterMode.AUTHORITY) | ||
public class ThemeManager extends BaseComponentSystem { |
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'd like to see a short class comment here explaining what this manager is about, and what a theme is supposed to be. Not critical for mergin though...
@In | ||
AssetManager assetManager; | ||
|
||
private Set<TownNameComponent> themeComponents = new HashSet<>(); |
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 are themeComponents
actually TownNameComponents
? Just out of convenience as the town name happens to contain information about the theme as well?
|
||
@Override | ||
public void postBegin() { | ||
logger.info("Loading default themes..."); |
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 whether we should reduce the log levels here... should those info
logs actually be debug
instead?
} | ||
} | ||
themeNames += "]"; | ||
logger.info("Finished loading themes: " + themeComponents.size() + " theme types found: " + themeNames); |
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.
The code above is just used for generating the log message, isn't it? I think we should remove it.
The following code using Stream API should yieldthe same result as your iterator-based code. The code is more declarative though which often helps to think more about the what than the how 😉
themeComponents
.stream()
.map(c -> c.name)
.collect(Collectors.joining(", ", "[", "]")
.toString()
public String name; | ||
public List<String> prefixes; | ||
public List<String> postfixes; | ||
public List<String> nameList; |
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 is nameList
supposed to be, given there are already name
and prefixes
and postfixes
? Is it some kind of tag list?
import org.terasology.entitySystem.systems.BaseComponentSystem; | ||
import org.terasology.entitySystem.systems.RegisterMode; | ||
import org.terasology.entitySystem.systems.RegisterSystem; | ||
import org.terasology.namegenerator.town.DebugTownTheme; |
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.
One thing: is the namegenerator
a transitive dependency? Do we need to add it to the module dependencies?
Game crashed right when I discovered the first city.... Edit: I did not have any culture activated... with MedievalCities in the active modules I can see that the city has a name when looking in the console, but there is no name tag showing in the world I would like to avoid a hard crash like the one that happens when you don't active an additional culture module, but the hardening against these case could also happen in another PR... Edit 2: Finally got a working version with name tags showing... @Cervator what's the difference between using MedievalCities game mode and just activating the modules on their own? The first works as intended, the second does not show name tags for me... |
Bump - @mayant15 you still out there? :-) |
The SiteFacetProvider no longer looks for sufficient grass when deciding if a site is suitable for building a city.
- Created basic container classes - Created temporary mock functions - Setup SettlementEntityManager to create roads as it creates buildings
- Buildings of the source will not be built upon roads - RoadSegments will be laid down better
Names are now assigned to sites on world generation as opposed to on encounter.
Closing this one, consider #46 instead. |
Solves #22 by generating city names.
To test: