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

Tilemap editor adds extra pixel for odd numbered tile resolutions #87138

Closed
IndieJoJo opened this issue Jan 13, 2024 · 3 comments · Fixed by #89929
Closed

Tilemap editor adds extra pixel for odd numbered tile resolutions #87138

IndieJoJo opened this issue Jan 13, 2024 · 3 comments · Fixed by #89929

Comments

@IndieJoJo
Copy link

Tested versions

Reproducible in Godot v4.2.1

System information

Godot v4.2.1.stable - Windows 10.0.19045 - Vulkan (Mobile) - dedicated AMD Radeon RX 6700 XT (Advanced Micro Devices, Inc.; 31.0.23013.1023) - AMD Ryzen 5 5600 6-Core Processor (12 Threads)

Issue description

When adding a tilemap and setting layers with Reset to default tile shape, odd numbered tile resolutions like (9x9 and 5x5) have an additional pixel added which can mess with physics and navigation.
image

Steps to reproduce

Try adding a layer and pressing reset to default tile shape for any odd numbered resolution.

Minimal reproduction project (MRP)

TilemapTest - Copy.zip

@kleonc
Copy link
Member

kleonc commented Jan 14, 2024

There are indeed truncations of floats to ints in few places resulting in incorrect offsets, PR fixing these incoming. AFAICT these affect only the debug drawing though.


What's a bigger problem is that NavigationServer2D::bake_from_source_geometry_data seems to produce incorrect navigation polygon.

Specifically here:

Ref<NavigationPolygon> nav_polygon;
nav_polygon.instantiate();
if (polygon_editor->get_polygon_count() > 0) {
Ref<NavigationMeshSourceGeometryData2D> source_geometry_data;
source_geometry_data.instantiate();
for (int i = 0; i < polygon_editor->get_polygon_count(); i++) {
Vector<Vector2> polygon = polygon_editor->get_polygon(i);
nav_polygon->add_outline(polygon);
source_geometry_data->add_traversable_outline(polygon);
}
nav_polygon->set_agent_radius(0.0);
NavigationServer2D::get_singleton()->bake_from_source_geometry_data(nav_polygon, source_geometry_data);

for a single outline [(-2.5, -2.5), (2.5, -2.5), (2.5, 2.5), (-2.5, 2.5)] it produces a navigation polygon with vertices [(3.0, 3.0), (-3.0, 3.0), (-3.0, -3.0), (3.0, -3.0)] (a single polygon with indices [0, 1, 2, 3]).

More specifically the rounding happens here when converting Vector2 to Point64:

const Vector<Vector2> &traversable_outline = p_navigation_mesh->get_outline(i);
Path64 subject_path;
for (const Vector2 &traversable_point : traversable_outline) {
const Point64 &point = Point64(traversable_point.x, traversable_point.y);

template <typename T2>
explicit Point<T>(const Point<T2>& p) { Init(p.x, p.y); }

template <typename T2>
inline void Init(const T2 x_ = 0, const T2 y_ = 0)
{
if constexpr (std::numeric_limits<T>::is_integer &&
!std::numeric_limits<T2>::is_integer)
{
x = static_cast<T>(std::round(x_));
y = static_cast<T>(std::round(y_));
}

No idea why it's using Point64 over PointD etc. cc @smix8

@smix8
Copy link
Contributor

smix8 commented Jan 24, 2024

That the 2D navigation mesh baking only works with integers and "full pixels" was a decision for performance and robustness.

In my early tests PointD was not only significantly slower (does internal integer remapping) but also had far more polygon errors and caused edge seam problems due to float errors.

That said that was a while ago so may be worth a retest with newer clipper versions if it is still such an issue using PointD. I really do not want to up and downscale the float positions manually like the old clipper1 required and Geometry2D still uses, that was very bad for performance.

@akien-mga
Copy link
Member

Fixed by #89929.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants