Implementation of scriptable sprite overlays. #120

MaxMahem · 2023-04-12T17:17:52Z

Okay while I could keep fiddling with this forever, I think it's ready for review.

Notes on my approach:

This introduces two new classes, InventoryItemSprite and SpriteOverlay.
InventoryItemSprite is similar to ItemSprite and UnitSprite. It takes on all responsibility for drawing InventoryItems/bigobs. It is non-owning and should not leave the local scope. This was necessary to cleanly provide a centralized way to enable or disable the rendering of various (now) optional behaviors. It also eliminates a small amount of code duplication. This also represents the biggest structural changes to the code since the logic that powered these effects is removed and moved into this class.
InventoryItemSprite depends on a small struct, InventorySpriteContext to inform the scripting engine of the current context in which the inventory sprite is being rendered and allows for the scripts to instruct the class to render or not-render the various options.
InventorySpriteContext also contains a set of static values representing the current OXCE behavior (Wound overlays are only drawn on units on the ground. Ammo counts do not show up in the inventory screen). I believe there is a good argument for changing some of these defaults, but I have left it as is for now. I considered exposing it to the rule files but decided it was too much work/too intrusive, and the behavior can be adjusted by scripting anyways.
InventoryItemSprite itself has no exposure to scripting, which is all handled by SpriteOverlay and InventorySpriteContext
Most scripting behavior is handled by free functions that have been friended, and tucked away in their own namespace. This lets them access private data where necessary but does not blot up the API with calls that are not relevant outside the scripting language.
SpriteOverlay is the primary interface for the scripting engine and holds most of the state information needed to perform the scripting (primarily the sprite bounds and the target surface).
Calling the scripting is straightforward, instance a new SpriteOverlay in the local scope (it should not leave the local scope and is non-owning of any resource) and then call the draw method on it. draw is a template function and providing the appropriate templates lets the appropriate scripts be called with the appropriate arguments.
There are 4 new scripting hooks added by this patch at this time:
- inventorySpriteOverlay which is called in the context of rendering an inventory sprite (anywhere it might be rendered, except the UFOpedia at this time). It exposes BattleItem data.
- handOverlay which is called in the context of rendering a hand-slot, which is commonly bigger than the sprite. Handslot sized render boxes are also used for the hover ammo box and items selected by the cursor, so it is also called in these contexts, though it has no effect by default. It exposes BattleItem data.
- unitPaperdollOverlay which is called when the paperdoll is rendered. It exposes Unit data.
- unitRankOverlay which is called when the unit rank is displayed on the battlescape.

I think that about covers it. I had other things in the past, but on further consideration, I backed off from them so this is pretty much only exactly what it says on the tin. There are a few additional scripting methods thrown in that I found necessary to generate the demo effects, which probably means they are generally useful regardless.

Anyways, I had fun making this, and am personally finding a lot of these enhancements useful.

MaxMahem · 2023-04-13T04:00:02Z

After some discussions with SupSuper I may have come up with an even better way of doing this, so I've converted this to a draft as I explore that.

Yankes

Initial batch of review

Yankes · 2023-04-13T08:41:22Z

src/Savegame/BattleUnit.cpp

@@ -5846,6 +5846,10 @@ void setFireScript(BattleUnit *bu, int val)
 	}
 }

+void getStatusScript(const BattleUnit* bu, int& status)
+{
+	status = bu ? bu->getStatus() : status;


result value should be deterministic, now is "random", set 0 or any other value when nullptr was given.

Yankes · 2023-04-13T08:43:46Z

src/Savegame/SavedBattleGame.cpp

@@ -3493,6 +3493,16 @@ void getTileScript(const SavedBattleGame* sbg, const Tile*& t, int x, int y, int
 	}
 }

+void getEnvrionmentShockIndicator(const SavedBattleGame* sbg, const Surface*& shockIndicator)


No guard for nullptr

Yankes · 2023-04-13T08:45:08Z

src/Savegame/SavedBattleGame.cpp

+	auto* enviro = sbg->getEnviroEffects();
+	auto* mod = const_cast<Mod*>(sbg->getMod());
+	shockIndicator = enviro && !enviro->getInventoryShockIndicator().empty() ? mod->getSurface(enviro->getInventoryShockIndicator(), false)
+																			 : mod->getSurface("BigShockIndicator", false);


Formatting (good to keep 80 line limit, especially when you have ?:)

Yankes · 2023-04-13T08:54:01Z

src/Mod/Mod.cpp

+*/
+void getInterfaceElementColor(const Mod* mod, int &color, const std::string& interfaceName, const std::string& elementName)
+{
+	auto interface = mod->getInterface(interfaceName);


none nullptr guard

Yankes · 2023-04-13T08:54:07Z

src/Mod/Mod.cpp

+ */
+void getInterfaceElementColor2(const Mod* mod, int& color, const std::string& interfaceName, const std::string& elementName)
+{
+	auto interface = mod->getInterface(interfaceName);


none nullptr guard

Yankes · 2023-04-13T08:55:26Z

src/Battlescape/SpriteOverlay.h

+class SavedBattleGame;
+
+/// Namespace for segregating SpriteOverlay scripting functions.
+namespace SpriteOverlayScript


why not simply make it member functions?

My thinking was twofold.

I didn't want to expose general consumption methods that probably shouldn't be called outside the scripting context. That is, all of these draw and blit methods really shouldn't be called by other C++ code. Such behavior is probably incorrect. But, for convenience's sake, I wanted to give them access to private data and avoid writing many pointless getter functions. friend-ing the class seemed to be the best way to compromise these two goals.

Less so for this method, but for the InventorySpriteContext, I was exposing enums in funky ways (as ints) for the scripting that I normally wouldn't. Again I wanted to conceal this from the general consumption code, so I applied the same method. These properties are public so it probably could have been just free functions instead, but I thought I would stick with this approach.

I used normal member functions in the (few) cases where I thought it was reasonable for both scripting and general code to use the same method.

But it could be private members, as ScriptRegister is part of class it can access private bits.

Didn't realize that. Rewrite with this in mind incoming.

Yankes · 2023-04-13T08:56:27Z

src/Battlescape/SpriteOverlay.cpp

+/// Draws a number on the surface the sprite targets.
+void drawNumber(SpriteOverlay* dest, int value, int x, int y, int width, int height, int color)
+{
+	dest->_numberRender.clear();


none nullptr guard

Yankes · 2023-04-13T08:56:34Z

src/Battlescape/SpriteOverlay.cpp

+/// Draws text on on the surface the sprite targets.
+void drawText(SpriteOverlay* dest, const std::string& text, int x, int y, int width, int height, int color)
+{
+	auto surfaceText = Text(width, height, dest->_bounds.x + x, dest->_bounds.y + y);


none nullptr guard

Yankes · 2023-04-13T08:58:50Z

src/Battlescape/InventoryState.cpp

+			const auto surfaceSet = *_game->getMod()->getSurfaceSet("BIGOBS.PCK", anim);
+			InventoryItemSprite(*firstAmmo, save, *_selAmmo, spriteBounds).draw(surfaceSet, InventorySpriteContext::SOLDIER_INV_AMMO, anim);
+
+			constexpr auto handSlotBounds = SDL_Rect{


check mods like WH40k use bigger hands items than normally allowed

Tested. The mod uses a custom sprite that fakes the appearance of a bigger box for hand items. The current behavior is maintained for these items, though this patch offers some options for handling customizations in these cases.

… drawing now.

MeridianOXC · 2023-04-22T06:15:41Z

update of src/OpenXcom.2010.vcxproj.filters is missing for display in Visual Studio

update of src/CMakeLists.txt is missing for compilation on Linux, Mac and Android

MaxMahem · 2023-04-23T03:43:49Z

Sorry for the spam, didn't mean to close this. Had to update the filter file manual, could not for the life of me get visual studio to do it.

Yankes · 2023-05-07T16:44:45Z

In week or two I will go back to this PR and analyze it in detail and possibly alter it a bit and merge manually.

Implementation of scriptable sprite overlays.

f44ef89

MaxMahem marked this pull request as draft April 13, 2023 03:59

Yankes requested changes Apr 13, 2023

View reviewed changes

Yankes self-assigned this Apr 13, 2023

Requested changes, also InventoryOverlay handles all inventory sprite…

a01ae51

… drawing now.

MaxMahem closed this Apr 23, 2023

MaxMahem deleted the temp branch April 23, 2023 02:37

MaxMahem restored the temp branch April 23, 2023 02:38

MaxMahem reopened this Apr 23, 2023

Requested changes to project files.

54855bf

MeridianOXC added the In review label May 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of scriptable sprite overlays. #120

Implementation of scriptable sprite overlays. #120

MaxMahem commented Apr 12, 2023

MaxMahem commented Apr 13, 2023

Yankes left a comment

Yankes Apr 13, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

MaxMahem Apr 13, 2023

Yankes Apr 13, 2023

MaxMahem Apr 21, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

Yankes Apr 13, 2023

MaxMahem Apr 21, 2023

MeridianOXC commented Apr 22, 2023

MaxMahem commented Apr 23, 2023

Yankes commented May 7, 2023

Implementation of scriptable sprite overlays. #120

Are you sure you want to change the base?

Implementation of scriptable sprite overlays. #120

Conversation

MaxMahem commented Apr 12, 2023

MaxMahem commented Apr 13, 2023

Yankes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeridianOXC commented Apr 22, 2023

MaxMahem commented Apr 23, 2023

Yankes commented May 7, 2023