Skip to content

Conversation

picobyte
Copy link
Contributor

@picobyte picobyte commented Mar 1, 2020

Fix two issues I noted in the sulis_editor:

an sulis_editor build requirement, mentioned in #169

  • One question, is it ok for fire_more_than_once to default to false, here?

and an out of bounds issue #172

@Grokmoo
Copy link
Owner

Grokmoo commented Mar 2, 2020

Something is off with the merge request as I don't see any changes? The most recent commit doesn't have anything to do with the editor?

@RoelKluin
Copy link

Sorry, will fix when I'm at home.

when changing race the index could become out of bounds

Signed-off-by: picobyte <[email protected]>
@picobyte picobyte closed this Mar 2, 2020
@picobyte picobyte force-pushed the sulis_editor_fixes branch from f9add76 to 2b9d4fe Compare March 2, 2020 18:48
@picobyte picobyte reopened this Mar 2, 2020
@picobyte
Copy link
Contributor Author

picobyte commented Mar 2, 2020

Just the #172 fix remains.

.selected_images
.insert(layer, (index, Rc::clone(&images_ref[index])));
}
let index = (window.selected_images.get(&layer).unwrap().0 - 1) % len;
Copy link
Owner

Choose a reason for hiding this comment

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

Because window.selected_images[&layer].0 is a usize, this will cause problems when it is 0. In particular, it panics when running in debug mode, and wraps around in release mode. Even when wrapping, I don't think the wrap around will be to the last entry either, but rather will be what seems like a random entry in the array.

I think the safest bet is to manually check for 0 and set it to len - 1 in this case.

@picobyte
Copy link
Contributor Author

picobyte commented Mar 3, 2020

Good catch, by adding len - 1 this can be solved without branches. Also, there was the danger of modulo 0 the images.is_empty() -> continue already prevents this.

@Grokmoo Grokmoo merged commit 26cc82b into Grokmoo:master Mar 4, 2020
@picobyte picobyte deleted the sulis_editor_fixes branch March 9, 2020 17:58
picobyte added a commit to picobyte/sulis that referenced this pull request Mar 11, 2020
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

3 participants