Hands-on Rust: can_place logic is "inside out"

[mikecargal]

Title: Hands-On Rust (Chapter 11: prefab)

Just played a couple of amulet-less games. With a bit of debugging, I believe that your can_place logic is inside-out. Current code starts with assumption that you can’t place and then set’s it to true if any of the positions work:

        let mut can_place = false;// (4)
        dimensions.for_each(|pt| {// (5)
            let idx = mb.map.point2d_to_index(pt);
            let distance = dijkstra_map.map[idx];
            if distance < 2000.0 && distance > 20.0 && mb.amulet_start != pt {// (6)
                can_place = true;
            }
        });

I think you need to start with a more positive attitude (you can_place unless you find something invalid):

        let mut can_place = true;
        dimensions.for_each(|pt| {
            let idx = mb.map.point2d_to_index(pt);
            let distance = dijkstra_map.map[idx];
            if distance >= 2000.0 || distance < 20.0 || pt == mb.amulet_start {
                can_place = false;
            }
        });

I’m also seeing some other weirdness:

• player stepping off into darkness,
• inaccessible floor tiles (I can see them diagonally)
• monsters on top of one another (pretty sure I’ve seen that)

I’m assuming the others are typos on my part and I have a good bit of debugging ahead of me. Will let you know what I find.

BTW, normally (i.e. in other environments I’ve coded in), every time something “weird” happened, I’d set up a unit test to catch it and then fix it (ensuring it’s never happen again). Would be interesting how unit testing i handled where there’s so much randomness involved.

[herbert]

I’ll do a big round of testing on can_place. Looking at it, I think you may be right - but I’ll have to test to be sure.

You really shouldn’t be able to step into darkness; the can_enter_tile function includes bounds checking to prevent you from leaving the map, and the current tile should always be lit. I’ll test a bit more, but I haven’t run into that one yet.

Inaccessible floor tiles can happen with these algorithms. I wonder if I can squeeze a solution in for that (if not, I’ll post it as an article on my website and include a link to it in the book). The easy solution is to use the same logic from find_most_distant that ensures a reachable tile is picked for the exit - by traversing a Dijkstra map and only allowing reachable tiles to be included. You can easily include a step to transform all unreachable tiles into walls.

I’ll add some test code for monsters on top of one another. It tries to avoid that!

Unit testing is a topic I’d love to have pages for; I hope to write a bit more on that at some point. Rust makes unit tests really easy. You can put tests in your regular code files and call cargo test to run all of your program’s unit tests. For example (from the geometry code in bracket-lib):

#[cfg(test)]
mod tests {
    use super::Point3;

    #[test]
    fn new_point3() {
        let pt = Point3::new(1, 2, 3);
        assert_eq!(pt.x, 1);
        assert_eq!(pt.y, 2);
        assert_eq!(pt.z, 3);
    }
}

Unit testing randomly/procedurally generated content becomes a story of testing for the things you don’t want to happen. So if you don’t want any unreachable walls, you’d write a test to check for them - and then make sure that your generator algorithm culls them. Keeping the test around is helpful, because it’ll catch the times you forgot to do that when you tweaked or added an algorithm. Unfortunately, it would be pretty easy to write a whole second book on unit testing!

[mikecargal]

Rustier: (??)

        let can_place = dimensions.point_set().iter().all(|pt| {
            let idx = mb.map.point2d_to_index(*pt);
            let distance = dijkstra_map.map[idx];
            distance < 2000.0 && distance > 20.0 && *pt == mb.amulet_start
        });

[mikecargal]

yeah, pretty sure stepping off the end of the world is a typo somewhere here. Need to track it down.

Good to know inaccessible tiles are a known thing. May try your fix.

I thought there was code to prevent monsters on monsters, so suspect it’s a mistake on my part. If I find otherwise I’ll let you know.

I know a bit about Rust unit testing in general… just seems like the things I’m running into would be tricky to set up unit tests for. I’m thinking of putting in some code that does some assertion checks and dumps info it it occurs.

Next couple of days are a bit “spoken for” so I’ll be less active for a few days.

[mikecargal]

Update on “stepping off end of world”…

It looks like it’s a combination of two things…

1 - in build_random_rooms() you have logic so that a room has to have a wall along the border of the world:

            if !overlap {
                room.for_each(|p| {
                    if p.x > 0 && p.x < SCREEN_WIDTH //.
                    && p.y > 0 && p.y < SCREEN_HEIGHT
                    {
                        let idx = map_idx(p.x, p.y);
                        self.map.tiles[idx] = TileType::Floor;
                    }
                });
                self.rooms.push(room)
            }

Whereas in drunkard, the drunkard can dig anywhere that is inbounds:

            if !map.in_bounds(drunkard_pos) {
                break;
            }

I changed this to:

            if !map.in_floor_bounds(drunkard_pos) {
                break;
            }

and added in_floor_bounds to map.rs:

    pub fn in_floor_bounds(&self, point: Point) -> bool {
        point.x >= 1 && //.
            point.x < SCREEN_WIDTH-1 && //.
            point.y >= 1 && //.
            point.y < SCREEN_HEIGHT-1
    }

also had to change the following in the while loop:

            self.drunkard(
                &Point::new(
                    rng.range(0, SCREEN_WIDTH),
                    rng.range(0, SCREEN_HEIGHT)
                ),
                rng,
                &mut mb.map
            );

to

            self.drunkard(
                &Point::new(
                    rng.range(1, SCREEN_WIDTH-1), 
                    rng.range(1, SCREEN_HEIGHT-1)
                ),
                rng,
                &mut mb.map,
            );

2 - This prevents the player from walking off into darkness, but only because he can’t walk into a wall that isn’t rendered.

(summary of next section: not rendering first row and first column)

I haven’t tracked down why the boundary tiles are not being rendered.
I’ve seen (well, not seen, but can tell they’re “there” from logging) unrendered wall on the top, the left, the right and bottom are “fine”, so it looks like I likely have some typo in the offset calculation that is showing everything down and right one position.

Off to see if I can find that bug…

[mikecargal]

About to have to step away, but found this interesting tidbit that may point to rendering issue.

I THINK there’s a bug in field_of_view_set() that doesn’t “see” x == 0 or y==0

In the fov.rs file, after

fov.visible_tiles = Some(field_of_view_set(*pos, fov.radius, map));

I added:

println!("FOV @{:?}={:?}",pos, fov.visible_tiles);

(NOTE: in both cases, I dumped the map to the console to verify there were walls along the top row and left side, and went to a floor tile at y ==1 or x == 1. I would expect their FOV sets to include the walls a x == 0 or y == 0). (Found a map with both, so player is at (1,1)

Map dump:

----------------------
Drunken Map 
----------------------
################################################################################
#...####............##....#.#....##.......####.....#.....###..#.....#.##.#######
###....#.#................#......###....#######........#.####..##........#######
#####...........##...............####....######..........####..####...#..#######
#######.##..#...###...###...##...........##......#..#......###..##....#..#######
######..........#...#........#..####.....##..#..........##.#....###..##..#####.#
#######....#......####..#.........###.#...#....##..#....##..#.###..........###.#
#########.##.......####.#.............###...............###....##..#..##..####.#
########........##..##......#...#.....###...##......##..####..####.#..###......#
#########........##..#......#...##....####..#..#..#.##.#####..####..#........###
##########.......###.....#........##..###......####.##.#####..#....##.....##.###
##########...##..####.......##....#.........#######.....####..##.#####....##.###
##########...##..###.........#..###...........##........###....#..#####..##..###
###########..#.....#....##...#..#...#...##....###....#..##....##.######......###
##########...#.##..#..............##....#....####..#....####.##..####.....######
#........##..#.###.....................####..####....####.........####....######
#######..........#........###...##.....####.####...######...###..####....#######
#####....#.....#..........##....##.#....###.#####....####.#####..###..##...#####
####...#####...###...........#...#.......##.###.....#####...###......#####..####
####....###..#.###.......##.##.......##.#...######..######.######...######..####
#####....##.#................#......###.#.########..###....#####..#.######..####
###......##......##......#######...###..#..######...###..######..#....###..#####
############.##..##......#######...###.##..#####..#..#...######....#####..######
###########..##..............##..#........#####..##..##########....#####.#######
###########......#...........###....#.###..########...#########....####..#######
#############...##....###.....#..####...@.###########.#########.#######.########
#############..###....####......########..###########.########...#####....######
##############..#.....#####......#######..########....#########....######...####
##########.............#####....#########....#####..######################...###
##########...##..#.##...######..###########.######..########################.#.#
#########..####.#..#..#..####........####...#####..#########################...#
#######...####.......###...#########.####..######..#########################..##
#######.#####..#..##..####.#########.#####.######..########################....#
#######....##.###..#....##.########..#####..###...##########################...#
##########.##...#....##.....########.######..##.############################...#
##########..###.###.####..#..###############....###########################..###
##########.###..###.###........#############...###########################..####
##########......###.####..##################...###########################A#####
##########.#..#####.####..###################..#################################
##########.#..#####.#####..###################.#################################
##########..#.....#.####...#################...#################################
##########...####.#.##....#################..###################################
###########..###.....####.##############....####################################
##########..####...######..############..#######################################
##########.#####.#########.###########..########################################
##########..####.#########...###################################################
###############..########....###################################################
################....##....######################################################
#################..#############################################################
################################################################################

(Got lucky with a top-right corner that demonstrates both)

Field of view for:

log (reformatted for readability)

FOV @Point { x: 1, y: 1 }=Some(
  {Point { x: 1, y: 1 }, 
   Point { x: 6, y: 3 }, 
   Point { x: 3, y: 2 }, 
   Point { x: 5, y: 3 }, 
   Point { x: 4, y: 1 }, 
   Point { x: 2, y: 2 }, 
   Point { x: 3, y: 1 }, 
   Point { x: 8, y: 4 }, 
   Point { x: 4, y: 2 }, 
   Point { x: 8, y: 3 }, 
   Point { x: 5, y: 2 }, 
   Point { x: 6, y: 2 }, 
   Point { x: 7, y: 3 }, 
   Point { x: 2, y: 1 }, 
   Point { x: 1, y: 2 }, 
   Point { x: 7, y: 4 }, 
   Point { x: 4, y: 3 }, 
   Point { x: 7, y: 2 }
})

[mikecargal]

Automata has same characteristics of being able to have a room on the boundary.

I worked out a HACK to ensure a wall around the entire map, but it was ugly.

Maybe “solution” is that it’s OK to have floors on the map boundary, and we just need to solve FOV bug??

[mikecargal]

re: “I’ll add some test code for monsters on top of one another. It tries to avoid that!”

I’m just not seeing anything that tries to prevent that (anymore at least).

movement.rs calls map.can_enter_tile()

but that just looks like this…

    pub fn can_enter_tile(&self, point : Point) -> bool {
        self.in_bounds(point) && self.tiles[map_idx(point.x, point.y)]==TileType::Floor
    }

So nothing about whether it’s already occupied.

Nothing other than movement.rs seems to take an interest in WantsToMove, so I’m just not seeing it.

Maybe I’ll take a crack at whether I can solve it myself on Thursday.

[mikecargal]

OK, this may be a bit “crude” and inefficient, but it seem to help prevent monsters from occupying the same spot (but not prevent it entirely as noted below).

In movement.rs:

add:

#[read_component(Point)]
#[read_component(Enemy)]

and immediately inside if map.can_enter_tile(want_move.destination) { I added:

        if !<&Point>::query()
            .filter(component::<Enemy>())
            .iter(ecs)
            .any(|pt| *pt == want_move.destination)
        {

This prevents monsters from moving to where a monster was at the start of the turn. However, I can still stack them by staging it such to force 2 monsters to want_move to the same destination. It looks like I need to work out some way to dedup the want_move.destinations in the command buffer, but I don’t see how to do that. And it appears that, since positions are not flushed between calls to the movement system for_each, I can’t see if there’s a monster already moved to where you want to move.

BTW…I added the following monster_monitor.rs system to track if we have two monsters at the same position:

use crate::prelude::*;
use std::collections::HashSet;

#[system]
#[read_component(Point)]
#[read_component(Enemy)]
pub fn monster_monitor(ecs: &mut SubWorld) {
    let mut set = HashSet::new();
    <&Point>::query()
        .filter(component::<Enemy>())
        .iter(ecs)
        .for_each(|point| {
            if set.contains(point) {
                println!("More than one monster @{:?}", point);
            }
            set.insert(point);
        });
}

and added .add_system(monster_monitor::monster_monitor_system()) right before the .build() calls in the systems/mod.rs functions. It gives me a heads-up when it happens (though I’ve gotten better at spotting it’ll happen)

Something is weird about it in that I always get each monster twice in the query if I hit the “rooms” map_builder… haven’t spotted what makes that happen yet. So it’s currently useless for the rooms map_builder.

[mikecargal]

Ah… needed to walk away for a few minutes…

In chasing.rs, before the movers.iter()... I added:

let out requested_destinations = HashSet::new();

then changed it if !attacked block to:

                if !attacked && !requested_destinations.contains(&destination) {
                    requested_destinations.insert(destination);
                    commands.push((
                        (),
                        WantsToMove {
                            entity: *entity,
                            destination,
                        },
                    ));
                }

I haven’t tested it extensively, but it seems to cut of these attempted moves at the source.

[mikecargal]

OK, I’m about to cry “uncle” on why

 <&Point>::query()
            .filter(component::<Enemy>())
            .iter(ecs)
            .count()

gives me twice the number of monsters when using the rooms builder, but is an exact match for the other builders (it has the position of each monster in the iter() twice).

I’ve checked, and I’m only spawning 19 monsters for rooms, and that query always return 38 entries. For the other Architects, it spawns 50 monsters and the query returns 50 items.

Kind of irritating because I’d like to have the “monster_monitor” running to check double check my code, but it reports all dups for the rooms architect.

Guess it’s time to move on, but curious if anyone sees the same behavior, or knows what is going on.

update: playing the game, it does seem that there are two monsters being placed into each room. I just can’t find where they’re being added (spawned) twice.

update to update: Found it!!

forgot to remove

        map_builder
            .rooms
            .iter()
            .skip(1)
            .map(|r| r.center())
            .for_each(|pos| spawn_monster(&mut ecs, &mut rng, pos));

from State::new in main.rs, when I added:

        map_builder.monster_spawns.iter().for_each(|pos| {
            println!("Spawning monster @{:?}", pos);
            spawn_monster(&mut ecs, &mut rng, *pos)
        });

Just did quick search of the text and I couldn’t find where we were instructed to add the block above (maybe something I already reported). If so, probably need to mention removing the old code.

[mikecargal]

I had to remove the old monster spawn from both the new() and the reset_game_state() methods. (These methods have both grown to contain a great deal of duplicate code. Locally, I did a refactoring to DRY up the repetitive code.

[mikecargal]

One last () comment.

This has turn into the feedback equivalent of a really bad pull request. Too many concerns under a single topic.

Summary:

• Logic on can_place() is “inside out”
• More than one monster can occupy the same position
• bug in field_of_view_set()
• (probably none issue) I went down a rathole where I thought that having a Floor tile on the edge of the world was an issue. That’s really only the case because of the field_of_view_set() issue, and it’s probably just fine to have a Floor tile on the edge of the world.

If you’d prefer, I can break these out into three separate entries, and we can ignore this stream-of-consciousness mess I’ve created.

[mikecargal]

Well… I guess we all knew that couldn’t be my last post on this thread.

I’ve been playing around trying to do things like removing all “magic values”, etc. I just now realized that, with the new code, I was never getting a prefab fortress placed.

So, I’ve given it another look. I think there are a few problems (beyond the “inside out” logic:

​const​ FORTRESS : (&str, i32, i32) = (​"​
​------------​
​---######---​
​---#----#---​
​---#-M--#---​
​-###----###-​
​--M------M--​
​-###----###-​
​---#----#---​
​---#----#---​
​---######---​
​------------​
​"​, 12, 11);”

Assumption: we want to try to place a fortress around the amulet guarded by the three ‘M’ monsters.

• when trying to find a placement we would want to know that the amulet position falls on a floor tile within the FORTRESS. There’s no check for that. (amulet could fall under a wall, outside the fortress walls, or on top of a monster)

For kicks, I’m going to make a run at trying to solve it. Not sure but what the “solution” may be more complicated than you bargained for at this point in the book. If I work it out, I’ll post what I come up with.

[mikecargal]

OK, here’s what I came up with. Assuming that we don’t want to change the Amulet position:

use crate::prelude::*;

struct FortressStruct<'a> {
    map_str: &'a str,
    x: i32,
    y: i32,
}

const OUTSIDE_FLOOR: char = '-';
const WALL: char = '#';
const MONSTER: char = 'M';
const POSSIBLER_AMULET_POS: char = '.';

const FORTRESS: FortressStruct = FortressStruct {
    map_str: "
------------
---######---
---#....#---
---#.M..#---
-###....###-
--M......M--
-###....###-
---#....#---
---#....#---
---######---
------------
",
    x: 12,
    y: 11,
};

pub fn apply_prefab(mb: &mut MapBuilder, rng: &mut RandomNumberGenerator) {
    let amulet_pos = mb
        .amulet_start
        .expect("Can't test placement without an amulet");

    let fortress_vec: Vec<char> = FORTRESS
        .map_str
        .chars()
        .filter(|a| *a != '\r' && *a != '\n')
        .collect();

    let mut amulet_offsets = Vec::new();
    let mut idx = 0;
    for y in 0..FORTRESS.y {
        for x in 0..FORTRESS.x {
            if fortress_vec[idx] == POSSIBLER_AMULET_POS {
                amulet_offsets.push(Point { x, y })
            }
            idx += 1;
        }
    }

    let valid_positions_around_amulet = amulet_offsets
        .iter()
        .map(|pt| amulet_pos - *pt)
        .filter(|pt| {
            pt.x > 0
                && pt.y > 0
                && pt.x + FORTRESS.x < SCREEN_WIDTH
                && pt.y + FORTRESS.y < SCREEN_HEIGHT
        })
        .collect::<Vec<Point>>();
    if valid_positions_around_amulet.len() == 0 {
        return;
    }
    let placement =
        valid_positions_around_amulet[rng.range(0, valid_positions_around_amulet.len())];
    let dimensions = Rect::with_size(placement.x, placement.y, FORTRESS.x, FORTRESS.y);
    let points = dimensions.point_set();
    mb.monster_spawns.retain(|pt| !points.contains(pt));

    #[cfg(debug_assertions)]
    println!("Prefab placed at {:?}", &placement);

    let mut i = 0;
    for ty in placement.y..placement.y + FORTRESS.y {
        for tx in placement.x..placement.x + FORTRESS.x {
            let idx = map_idx(tx, ty);
            match fortress_vec[i] {
                POSSIBLER_AMULET_POS
                    if mb.map.index_to_point2d(idx) == mb.amulet_start.unwrap() =>
                {
                    ()
                }
                POSSIBLER_AMULET_POS | OUTSIDE_FLOOR => mb.map.tiles[idx] = TileType::Floor,
                MONSTER => {
                    mb.map.tiles[idx] = TileType::Floor;
                    mb.monster_spawns.push(Point::new(tx, ty));
                }
                WALL => mb.map.tiles[idx] = TileType::Wall,
                c => println!("No idea what to do with [{}]", c),
            }
            i += 1;
        }
    }
}

The idea is to calculate all the possible valid positions (where the amulet is on a floor tile inside the fortress), and then randomly select one (if there are any). For the Drunkard and Automata maps, my experience is that the amulet most often lands too close to the edge of the world to have any valid positions.

It would probably be simpler to just manage to get the fortress over the Amulet and re-position the amulet to the center of the fortress, but I’m probably going to keep this one. Doing it this way, there would always be a way to position the fortress, so we could just choose to position it x% of the time.

[herbert]

Hey, sorry about the slow replies. I’ve been really rather unwell, and slept through the last week and a half or so. I’ll go over this thread today and turn it into action items. I’m going to have to strike a balance between the focus of the book (teaching Rust/gamedev) and making the procgen content a bit more solid. I’ll get back to you soon with some ideas - thanks for taking the time to look at it all.

[mikecargal]

Yeah, after spending more time just enhancing things to learn more Rust, I’m definitely seeing the trade off with focus on book topics, and going down side topics too much. Don’t envy you trying to find the right balance.

[mikecargal]

Got bored and decided I’d take a crack at finding why the top row and first column weren’t being rendered.

Learned how to use [patch.crates.io] to reference my local repo… so, that’s good.

Even better… the bug is no longer present in the master branch. (so not much of a debugging opportunity )

(Though something odd must be going on in that I had to bump the version on smallvec to get a good build, so “master” must not be what’s published to crates.io (I see that smallvec was removed on a couple of other branches, didn’t try to sort it all out)

[herbert]

The “master” build on github is going through heavy testing, with a view to pushing a bug-fix release very soon. Waiting on a winit update that fixes a bunch of Big Sur issues to try and include them at the same time.

[mikecargal]

The “odd” part (to me) was that the master branch had version “~1.4.0”, but I needed to bump it to “~1.5.0” to get compatibility with the legion version. So, it seems that the master branch (that your testing now, is behind a version from what’s on crates.io (???)

Or… (duh) maybe what’s on crates.io doesn’t use smallvec ( ). If that’s the case, you might want to make sure you’ve bumped to 1.5 on the master branch.