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

Added new scatter plot #157

Merged
merged 15 commits into from
Oct 31, 2015
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion tests/draw_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import os
from worldengine.draw import _biome_colors, Image, draw_simple_elevation, elevation_color, \
draw_elevation, draw_riversmap, draw_grayscale_heightmap, draw_ocean, draw_precipitation, \
draw_world, draw_temperature_levels, draw_biome
draw_world, draw_temperature_levels, draw_biome, draw_scatter_plot
from worldengine.biome import Biome
from worldengine.world import World

Expand Down Expand Up @@ -162,5 +162,10 @@ def test_draw_biome(self):
draw_biome(w, target)
self._assert_img_equal("biome_28070", target)

def test_draw_scatter_plot(self):
Copy link
Member

Choose a reason for hiding this comment

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

this tests just that it does not explode, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a coverage test. It tests pretty much all of the new code (since the world being used contains all temperature and humidity ranges).

w = World.open_protobuf("%s/seed_28070.world" % self.tests_data_dir)
target = PixelCollector(16, 16)
draw_scatter_plot(w, 16, target)

if __name__ == '__main__':
unittest.main()
15 changes: 14 additions & 1 deletion worldengine/cli/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from worldengine.common import array_to_matrix, set_verbose, print_verbose
from worldengine.draw import draw_ancientmap_on_file, draw_biome_on_file, draw_ocean_on_file, \
draw_precipitation_on_file, draw_grayscale_heightmap_on_file, draw_simple_elevation_on_file, \
draw_temperature_levels_on_file, draw_riversmap_on_file
draw_temperature_levels_on_file, draw_riversmap_on_file, draw_scatter_plot_on_file
from worldengine.plates import world_gen, generate_plates_simulation
from worldengine.imex import export
from worldengine.step import Step
Expand Down Expand Up @@ -78,6 +78,10 @@ def generate_rivers_map(world, filename):
draw_riversmap_on_file(world, filename)
print("+ rivers map generated in '%s'" % filename)

def draw_scatter_plot(world, filename):
draw_scatter_plot_on_file(world, filename)
print("+ scatter plot generated in '%s'" % filename)


def generate_plates(seed, world_name, output_dir, width, height,
num_plates=10):
Expand Down Expand Up @@ -302,6 +306,8 @@ def main():
g_generate.add_argument('--not-fade-borders', dest='fade_borders', action="store_false",
help="Not fade borders",
default=True)
g_generate.add_argument('--scatter', dest='scatter_plot',
Copy link
Member

Choose a reason for hiding this comment

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

perhaps it would make more sense to have it as a separate operation, like drawing the ancient map. We could also want to change the name because scatter plot is a type of plot but it does not tell us what kind of information is being represented. Later we could have differe scatter plots representing different things

Copy link
Member

Choose a reason for hiding this comment

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

We could leave it as an option of the generation process for now but just change the name

action="store_true", help="generate scatter plot")
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this necessitate updating README.md? I would prefer it.


# -----------------------------------------------------
g_ancient_map = parser.add_argument_group(
Expand Down Expand Up @@ -417,6 +423,9 @@ def main():
if args.rivers_map and not generation_operation:
usage(error="Rivers map can be produced only during world generation")

if args.scatter_plot and not generation_operation:
usage(error="Scatter plot can be produced only during world generation")

print('Worldengine - a world generator (v. %s)' % VERSION)
print('-----------------------')
print(' operation : %s generation' % operation)
Expand All @@ -431,6 +440,7 @@ def main():
print(' step : %s' % step.name)
print(' greyscale heightmap : %s' % args.grayscale_heightmap)
print(' rivers map : %s' % args.rivers_map)
print(' scatter plot : %s' % args.scatter_plot)
print(' fade borders : %s' % args.fade_borders)
if operation == 'ancient_map':
print(' resize factor : %i' % args.resize_factor)
Expand Down Expand Up @@ -458,6 +468,9 @@ def main():
if args.rivers_map:
generate_rivers_map(world,
'%s/%s_rivers.png' % (args.output_dir, world_name))
if args.scatter_plot:
draw_scatter_plot(world,
'%s/%s_scatter.png' % (args.output_dir, world_name))

elif operation == 'plates':
print('') # empty line
Expand Down
82 changes: 82 additions & 0 deletions worldengine/draw.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,6 +406,83 @@ def draw_biome(world, target):
v = biome[y, x]
target.set_pixel(x, y, _biome_colors[v])

def draw_scatter_plot(world, size, target):
""" This function can be used on a generic canvas (either an image to save
on disk or a canvas part of a GUI)
"""
min_humidity = None
max_humidity = None
min_temperature = None
max_temperature = None
for y in range(world.height):
Copy link
Contributor

Choose a reason for hiding this comment

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

    min_humidity = world.humidity['data'].min()
    max_humidity = world.humidity['data'].max()
    min_temperature = world.temperature['data'].min()
    max_temperature = world.temperature['data'].min()

This can replace the loops between line 417 and 429. (And, of course, lines 413 to 416.)

for x in range(world.width):
if world.is_land((x, y)):
t = world.temperature_at((x, y))
p = world.humidity['data'][y, x]
if min_temperature is None or t < min_temperature:
min_temperature = t
if max_temperature is None or t > max_temperature:
max_temperature = t
if min_humidity is None or p < min_humidity:
min_humidity = p
if max_humidity is None or p > max_humidity:
max_humidity = p

temperature_delta = max_temperature - min_temperature
Copy link
Contributor

Choose a reason for hiding this comment

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

You seem to have included this twice now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Thanks. Fixed.

humidity_delta = max_humidity - min_humidity

for y in range(0, size):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind adding comments to every step of the preparation? Just two words to make the code more digestible. Somebody will come after you at some point. ;)

for x in range(0, size):
target.set_pixel(x, y, (255, 255, 255, 255))
for t in range(0, 6):
v = (size - 1) * ((world.temperature['thresholds'][t][1] - min_temperature) / temperature_delta)
for y in range(0, size):
target.set_pixel(int(v), y, (0, 0, 0, 255))
ranges = ['87', '75', '62', '50', '37', '25', '12']
for p in ranges:
h = (size - 1) * ((world.humidity['quantiles'][p] - min_humidity) / humidity_delta)
for x in range(0, size):
target.set_pixel(x, int(h), (0, 0, 0, 255))

for y in range(world.height):
for x in range(world.width):
if world.is_land((x, y)):
t = world.temperature_at((x, y))
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what the "convention" is, but I prefer world.temperature['data'] - it saves a ton of function calls and looks a lot more in line with world.humidity['data'], which is just prettier.

Copy link
Member

Choose a reason for hiding this comment

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

actually I would prefer temperature_at: the fact there is an array named data inside the field temperature is an implementation details and it is better to avoid relying on that when it makes sense to use something slightly more declarative

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmh, ok. I was mostly bothered by it since it doesn't look homogeneous.

p = world.humidity['data'][y, x]
if world.is_temperature_polar((x, y)):
r = 0
elif world.is_temperature_alpine((x, y)):
r = 42
elif world.is_temperature_boreal((x, y)):
r = 85
elif world.is_temperature_cool((x, y)):
r = 128
elif world.is_temperature_warm((x, y)):
r = 170
elif world.is_temperature_subtropical((x, y)):
r = 213
elif world.is_temperature_tropical((x, y)):
r = 255
if world.is_humidity_superarid((x, y)):
b = 32
elif world.is_humidity_perarid((x, y)):
b = 64
elif world.is_humidity_arid((x, y)):
b = 96
elif world.is_humidity_semiarid((x, y)):
b = 128
elif world.is_humidity_subhumid((x, y)):
b = 160
elif world.is_humidity_humid((x, y)):
b = 192
elif world.is_humidity_perhumid((x, y)):
b = 224
elif world.is_humidity_superhumid((x, y)):
b = 255
nx = (size - 1) * ((t - min_temperature) / temperature_delta)
ny = (size - 1) * ((p - min_humidity) / humidity_delta)
target.set_pixel(int(nx), int(ny), (r, 128, b, 255))


# -------------
# Draw on files
Expand Down Expand Up @@ -477,3 +554,8 @@ def draw_ancientmap_on_file(world, filename, resize_factor=1,
draw_biome, draw_rivers, draw_mountains, draw_outer_land_border,
verbose)
img.complete()

def draw_scatter_plot_on_file(world, filename):
img = ImagePixelSetter(512, 512, filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the hard-coded values? What happens to non-square maps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are plots, not maps. They allow you to see the relationship between temperature and rainfall so you can be sure that the values are too far out of bounds. As such there is no reason for the size of the plot to be related to the size of the map (and since I generate maps that are 2k and 4k I really don't want plots that large).

I could add arguments so that there's a completely separate X and Y value for the scatter plot but it seems like an excessive complication that would lead to occasional user errors. Not many, granted, but it would still probably cause more problems than it resolves.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the plot is purely used for informational purposes, I guess it might be ok. If others are fine with it, then do it the way you like.
(I love the huge maps, however.^^)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I love huge maps as well. but it just makes no sense to make the plot huge. I'm going to include an example plot below so you can see what it looks like (I will probably make some changes in the future such as labeling the axis, but for now this should suffice).

draw_scatter_plot(world, 512, img)
Copy link
Member

Choose a reason for hiding this comment

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

but draw_scatter_plot doesn't take just 2 parameters?
perhaps we could use constants to make it clear the meaning of the value 512. It is absolutely fine to have a chart with fixed size, it should be just clear that the first 512x512 means the chart is 512 pixels wide and tall

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. That's a little bit of future proofing. While I think it would be a mistake to make it so that the size can be changed I didn't want to hardwire it in case I missed something and there was a compelling reason).

This way if I'm wrong it requires much less in the way of code change.

img.complete()
3 changes: 2 additions & 1 deletion worldengine/generation.py
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ def generate_world(w, step):
if not step.include_precipitations:
return w

TemperatureSimulation().execute(w, seed)
# Precipitation with thresholds
PrecipitationSimulation().execute(w, seed)

Expand All @@ -212,7 +213,7 @@ def generate_world(w, step):

# FIXME: create setters
IrrigationSimulation().execute(w, seed)
TemperatureSimulation().execute(w, seed)

Copy link
Contributor

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 actually changes anything. Did you move the line on purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't change anything just yet, but it will be changing things later on. Currently we are modifying the amount of rainfall based on the latitude. This is because areas with lower temperatures tend to experience lower rainfall (on average. There's lots of other weather effects as well but its not a bad starting rule and we are already using it, simply indirectly). To modify rainfall so it considers temperature means we have to move TemperatureSimulation to be triggered before PrecipitationSimulation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the clarification.
This is the change that minorly interferes with #146 , so you may have to redo this should #146 be merged first (but redoing should be trivial).

Copy link
Contributor

Choose a reason for hiding this comment

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

Once you rebase to master, these changes will have to be redone. But swapping out two lines shouldn't be an insurmountable task.^^

HumiditySimulation().execute(w, seed)


Expand Down