Alberth wrote:

The tight while loops that look for a next sane sprite may add loads of insane sprites to the small map, which take up storage, but are never edited.
Similarly, a user may 'walk' through the sprites looking for the right one to edit, and add lots of good sprites that way.

Instead of adding a sprite the first time you encounter it, would it be an idea to add it the first time the offset gets changed?
At least you only have truly edited sprites in the smallmap then.

I am not sure we should store the modifications at all, but don't have a good alternative either at this time.

This comment was imported from FlySpray: https://bugs.openttd.org/task/6236#comment13785

juzza1 wrote:

Your approach certainly makes a lot more sense than mine. I implemented your suggestion by moving the part where the mapping gets updated to the switch cases of up, down, left and right arrow buttons - though I'm not sure if this is the best way to detect a change in the offset. The wrapper function for updating the map is not needed anymore. When the relative offsets are reset, the SpriteID is now removed from the map. 0, 0 is shown for the relative offsets if the sprite is not found in the map. I also changed the xy_offs typedef name to CamelCase (not sure if this is the right coding style?)

Attachments

• sprite_aligner_relative_offsets_v2.patch (7.88 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6236#comment13787

Alberth wrote:

Index: src/lang/english.txt

STR_SPRITE_ALIGNER_SPRITE_TOOLTIP :{BLACK}Representation of the currently selected sprite. The alignment is ignored when drawing this sprite
STR_SPRITE_ALIGNER_MOVE_TOOLTIP :{BLACK}Move the sprite around, changing the X and Y offsets
-STR_SPRITE_ALIGNER_OFFSETS :{BLACK}X offset: {NUM}, Y offset: {NUM}
+STR_SPRITE_ALIGNER_OFFSETS :{BLACK}X offset: {NUM}, Y offset: {NUM} (Absolute)

I think it's better to add a new string for the 'absolute' line, instead of re-using the old line. You changed the meaning, which may invalidate the entire translation.

Index: src/newgrf_debug_gui.cpp

struct SpriteAlignerWindow : Window {
SpriteID current_sprite; ///< The currently shown sprite
Scrollbar *vscroll;
+ typedef SmallPair<int16, int16> XyOffs;
+ SmallMap<SpriteID, XyOffs> offs_start_map; ///< Mapping of starting offsets for the sprites which have been moved in the sprite aligner window

Move the 'typedef' out of the variables (below the 'struct SpriteAlignerWindow' line, for example), so it is more clearly visible.
The line also needs some doxygen comment.

===================================================================

In general, all comments should end with a "." (ie they are normal English sentences).

===================================================================

+ * Show 0 if current sprite is not yet in offset mapping
+ */
+ if (this->offs_start_map.Contains(this->current_sprite)) {
+ const XyOffs old_xy = this->offs_start_map.Find(this->current_sprite)->second;

First checking for containment, and then finding it sounds like you're searching the map twice. Can you combine these searches?
Most 'Find' operations in maps can return 'not found' value, which you can use to select the 'else' part.

+ SetDParam(0, -(old_xy.first) + spr->x_offs);
+ SetDParam(1, -(old_xy.second) + spr->y_offs);

-(A) + B can be written as B - A, which reads much nicer.

+ } else {
+ SetDParam(0, 0);

===================================================================

+ /* Remember the original offsets of the current sprite, if not already in map */
+ if (!(this->offs_start_map.Contains(this->current_sprite))) {
+ const Sprite *spr = GetSprite(this->current_sprite, ST_NORMAL);
+ this->offs_start_map.Insert(this->current_sprite, XyOffs(spr->x_offs, spr->y_offs));
+ }

Not in the patch, but just a little below, the same "GetSprite" operation is done again. Can't you combine both calls? It looks like it's possible.

===================================================================

  		NWidget(NWID_HORIZONTAL), SetPIP(10, 5, 10),
  			NWidget(WWT_LABEL, COLOUR_GREY, WID_SA_OFFSETS), SetDataTip(STR_SPRITE_ALIGNER_OFFSETS, STR_NULL), SetFill(1, 0),
  		EndContainer(),

+ NWidget(NWID_HORIZONTAL), SetPIP(10, 5, 10),
+ NWidget(WWT_LABEL, COLOUR_GREY, WID_SA_OFFSETS_REL), SetDataTip(STR_SPRITE_ALIGNER_OFFSETS_REL, STR_NULL), SetFill(1, 0),
+ EndContainer(),

A horizontal row with just one widget seems like a bit of a waste. Only keep the WWT_LABEL line, adding "SetPadding(0, 10, 0, 10)," to handle the "SetPIP" call.
(and yep, the same holds for the 3 lines above it :) )

===================================================================

Squirrel widgets need to be updated as well.
(src/script/api/game/game_window.hpp.sq, there are scripts in src/script/api)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6236#comment13788

juzza1 wrote:

Here is a new patch with all the recommended improvements. I also changed the "Reset Relative" button widget to use SetFill(0, 0,) instead of SetResize(0, 0), as the other text buttons also use this property. I cannot say I completely understand how the widgets work, so not sure which one to use? Both seem to have the same effect, at least on normal size interface.

Some comments I previously added used different terms for sprite alignment (moving vs aligning) and for mapping (map vs mapping)- these have been unified.

Attachments

• sprite_aligner_relative_offsets_v3.patch (9.61 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/6236#comment13790

Alberth closed the ticket.

Reason for closing: Implemented

In r27174, thanks for the patch

This comment was imported from FlySpray: https://bugs.openttd.org/task/6236