search

AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.41k stars 290 forks source link

Tile Palette Selection Not Reset When Palette Rearranged #690

Closed helle253 closed 5 years ago

helle253 commented 5 years ago

Summary

It was observed that, when the tile palette is rearranged during window resizing, the selection rectangle is not reset, even when the tiles within the selection are changed due to the resize. When attempting to place tiles using the palette afterwards, it uses a different set of tiles than the once previously used.

How to reproduce

  1. Select some tiles in the tile palette on the right side of the palette
  2. Resize the palette such that some or all of the tiles within the selection are moved to better fit the new window.
  3. Place more tiles. Observe that the selected tiles have changed.

Workaround

Don't resize the tile palette while using it to place tiles.

Analysis

When resizing, the location of the tiles change. Since the selection doesn't reset in this instance, it gets the tile that would now follow the end of the selection.

e.g. if your selection now selects (0, x+1), where x is the end of the available tiles, and x+1 is blank due to the resize, the tile palette uses (1,0), looping back around to the first column of the next row.

One of two things should happen to fix this:

  1. Naively remove the selection whenever the window is resized.
  2. When resizing, scan for whether tiles in the selection are affect, and remove the selection when they are.

Attachments

2018-10-06-08-13-15

Pooch11 commented 5 years ago

Is this issue still open? or related to #689 and resolved

helle253 commented 5 years ago

This is still an issue. I don't believe any fixes have been made regarding this.

On Thu, Oct 25, 2018 at 12:28 PM Adam notifications@github.com wrote:

Is this issue still open? or related to #689 https://github.com/AdamsLair/duality/issues/689 and resolved

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/AdamsLair/duality/issues/690#issuecomment-433137770, or mute the thread https://github.com/notifications/unsubscribe-auth/AQDL5THYs2qtt20kozJOooLOJpXIA94xks5uofSogaJpZM4XLVwI .

-- Nathan Heller Software Engineer Abbott Labs 1 St Jude Medical Dr, St Paul, MN 55117 (715) 551-3074

ghost commented 5 years ago

Where should I look first to start resolving this issue @ilexp ?

ilexp commented 5 years ago

The selection logic is done in the tile palette, most of the code for this is probably going to be there. View only behavior is derived from the tileset view.

chaosTechnician commented 5 years ago

Hey all, I'm new here. I've taken a quick look at this particular issue, and I believe I have enough understanding to provide a quick fix. I'm trying things out locally right now and making adjustments.

But, I'm not actually able to repro the issue in source from master; it looks like changes in SourcePaletteTilesetView::OnTileDisplayModeChanged clear the selection if it's affected by tile reorganization. Is addressing the selection change mentioned in this bug actually still open and up-for-grabs?

ilexp commented 5 years ago

Yes, the issue still persists and it's currently up for grabs.

The code part you mentioned does look like it should take care of the issue, but it seems like this isn't the case - reproduces just like in the gif at the top. Maybe you can look into why exactly that is?

Edit: Note on the repro case, I just ran the Tilemaps sample, created a new Tilemap from the included Tileset, and then performed selection and palette resize as shown above. If you're having trouble reproducing this, can you share how your (non-)repro case looks like?

Edit: On second thought, the code you mentioned doesn't clear or change the selection at all - it only recalculates the selected tile buffer in the new view, and it doesn't account for potential out of bounds selections. This would be a good spot to start looking into a potential fix though!

cowmanjoe commented 5 years ago

I just took a look into this issue and found that I could not reproduce the bug. Sorry to ask for the third time in this thread but is this issue still open? Are there any other additional steps that might be needed to reproduce this? Hopefully this gif will be visible enough to see that the same bug does not occur on my screen.

This behaviour still seems undesirable though. I think it still makes sense to modify it in one of the ways outlined above:

1.Naively remove the selection whenever the window is resized.

  1. When resizing, scan for whether tiles in the selection are affect, and remove the selection when they are.

Let me know if it is still needed that one of these two fixes happens, and if one is strongly more desirable than the other.

ilexp commented 5 years ago

I just took a look into this issue and found that I could not reproduce the bug. Sorry to ask for the third time in this thread but is this issue still open?

I just checked with the latest master and the issue is still there. I'm not sure why it doesn't seem to reproduce in some cases.

Are there any other additional steps that might be needed to reproduce this?

None that I know of.

  1. Run the tilemaps sample.
  2. Dragdrop the tilemap into the tilemap editor, or create a tilemap by any other means.
  3. Edit tiles as shown in this gif.

As far as I can tell, that's already what you're doing, only with a different tileset. Can you check if you can reproduce the behavior with the sample tileset?

Let me know if it is still needed that one of these two fixes happens, and if one is strongly more desirable than the other.

The second solution sounds a bit nicer, since it is non-destructive. The first one might negatively affect regular workflows.

cowmanjoe commented 5 years ago

Thanks for the reply. Is the tilemap sample somewhere in the repository or online? I'm new to Duality so I'm not sure where to find things.

ilexp commented 5 years ago

If you check out the repository and build the Duality solution, you should find a Tilemaps sample project under the samples folder. Set it as a startup project and hit Debug - the Duality editor should open up with the Tilemaps sample.

cowmanjoe commented 5 years ago

Didn't notice the sample inside the repo! I could reproduce with that for whatever reason. Think the check added in #722 solves the issue. I'm not sure if it's the proper place to put that logic, but it seemed reasonable to me.

ilexp commented 5 years ago

Fixed by @cowmanjoe in PR #722. Closing this.