Token Footprint Functions and Campaign Properties

ColdAnkles commented 2 months ago

Identify the Bug or Feature request

Basic Implementation of #4849, submitted for review

Description of the Change

Moves Token Footprints from being Static and Part of Campaign Properties. MTScript functions for getting and setting Footprints

Possible Drawbacks

Initial naive implementation, possibly with some weirdness from my dev environment

Documentation Notes

getTokenFootprints() getTokenFootprints(gridType) getTokenFootprints(gridType, footprintName)

getGridTypes()

getFootprintNames() getFootprintNames(gridType)

removeTokenFootprint(gridType, footprintName)

setTokenFootprint(gridType, footprintName, JSONFootPrintData)

resetFootprintsToDefault()

Release Notes

Custom Token Footprint Support

This change is

cwisniew commented 2 months ago

The tests this fails will also need to be fixed up

ColdAnkles commented 2 months ago

Agreed on the UI front, but it's not something I'm very good at, so since @bubblobill offered to do the GUI in #4849 if I got the backend, this PR is solely for backend enablement of the incoming GUI portion. The macro parts were mostly because I'm capable of implementing them, and I needed something to help test with. (and they'd be desired functionality anyway). I also added a quick and dirty mitigation for #4856 - though would like something better (as in generating a custom footprint that appears to cover the approximate area). Needs more thought.

On the tests failing: I've taken a look and can't see anything obvious between what I've changed and which tests fail

cwisniew commented 2 months ago

On the tests failing: I've taken a look and can't see anything obvious between what I've changed and which tests fail

Taking a quick look my guess would be the calls to CampaignProperties.loadFootprints(), the XML probably not loading correctly as its getting xstream errors. Try remove the localizedName in the classe (which shouldn't be saved anyway) and see if that fixes the issue. Or if you need that variable put transient in front of it so it is not persisted

you can run the tests locally with .\gradlew.bat test. (or ide) to test

bubblobill commented 2 months ago

I noticed you didn't include the grid type "isometric hex". One day someone is going to implement it beyond including it in the code add as an option. :)

cwisniew commented 2 months ago

I noticed you didn't include the grid type "isometric hex". One day someone is going to implement it beyond including it in the code add as an option. :)

Thanks for volunteering... Just know I believe in you @bubblobill :)

ColdAnkles commented 2 months ago

On the tests failing: I've taken a look and can't see anything obvious between what I've changed and which tests fail

Taking a quick look my guess would be the calls to CampaignProperties.loadFootprints(), the XML probably not loading correctly as its getting xstream errors. Try remove the localizedName in the classe (which shouldn't be saved anyway) and see if that fixes the issue. Or if you need that variable put transient in front of it so it is not persisted

you can run the tests locally with .\gradlew.bat test. (or ide) to test

Okay, its definitely the footprint loading - removing the localizedName variable or making it transient didn't make a difference, but omitting all the calls to footprint loading from the initFootprints step did. Not sure what to try next.

At any rate: the reason for my choices with localizedName existing separate to name is to allow for a framework to define sizes/footprints and have the display/localized name overwritten freely by someone else without effecting ability to address footprints by their "true" name - and also not having to make said localization dependant on MapTool - though it does work on the assumption that servers and clients are working in the same locale. Also it doesn't work as intended yet.

cwisniew commented 2 months ago

Okay, its definitely the footprint loading - removing the localizedName variable or making it transient didn't make a difference, but omitting all the calls to footprint loading from the initFootprints step did. Not sure what to try next.

There is probably no good reason to have default footprints in external serialised files in any case, probably just make classes with the default values

At any rate: the reason for my choices with localizedName existing separate to name is to allow for a framework to define sizes/footprints and have the display/localized name overwritten freely by someone else without effecting ability to address footprints by their "true" name - and also not having to make said localization dependant on MapTool - though it does work on the assumption that servers and clients are working in the same locale. Also it doesn't work as intended yet.

This is not a hreat assumption though there will be cases where client is different from server. Really it should be something that is localised within MT and users don't need to localise themselves

ColdAnkles commented 2 months ago

This is not a hreat assumption though there will be cases where client is different from server. Really it should be something that is localised within MT and users don't need to localise themselves

including localizations for every possible word/phrase people might choose to name a token size?

cwisniew commented 2 months ago

This is not a hreat assumption though there will be cases where client is different from server. Really it should be something that is localised within MT and users don't need to localise themselves

including localizations for every possible word/phrase people might choose to name a token size?

Ah gotchya, it's a super supplied name? Then that is fine

ColdAnkles commented 2 months ago

Okay, so excising the XML based defaults and replacing with code based works fine and tests pass.

However I've noticed that some footprints on hex grids are busted, and I can't see how my changes effect this

bubblobill commented 2 months ago

There are about 5 principal methods of referencing grids, each with a zillion variants. The one we use is probably the least useful for compatibility and interoperability between grid types with the possible exception of polar coordinates. Every time I considered doing what you are doing, I baulked because the xml makes no sense.

ColdAnkles commented 2 months ago

Here's what I'm seeing re: Hex Grid weirdness

Standard Large footprint on both hex grids do this kind of thing, yet the coordinates in the footprint are unchanged

bubblobill commented 2 months ago

Didn't someone recently fix something relating to footprints? Maybe in #4830?

ColdAnkles commented 2 months ago

It's definitely something related to my changes - develop and 1.14.3 don't have the issue

bubblobill commented 2 months ago

Swap your vertical and horizontal hex logic.

ColdAnkles commented 2 months ago

Swap your vertical and horizontal hex logic.

I'm not following (unless you meant to swap makeVertHex and makeHorizHex in which case I did, and had no effect.)

bubblobill commented 2 months ago

The hex that is out of place could have resulted from being calculated using the wrong grid type. Was hoping it was an easy fix due to a bad offset calc.

ColdAnkles commented 2 months ago

as best I can tell, for both grids, every second row of cells has off-by-one coordinates and I'm a little lost on where to go find the source of the issue

bubblobill commented 2 months ago

This is correct. I thought your problem may be the you are getting the offset on the wrong row/column. Try incrementing it by one and see if the shape ends up correct. You may simply be getting even offsets on odd rows and vice versa.

bubblobill commented 2 months ago

Go here and follow the link to redBlobGames. Read his stuff on hex grids.

https://github.com/RPTools/maptool/wiki/On-Maps,-Grids,-Objects,-and-Location

ColdAnkles commented 2 months ago

Assuming my understanding of hex grids is functional, here's a better diagram from what I'm observing: What's up with (-1,-1) under actual? That entire row is off by one.

Baaaaaz commented 2 months ago

May need to adjust your expectation! ;)

MapTool seems to actually use the following coordinates for pointy-top hex grids:

and for flat-top hex grids:

These align with the hex "odd-r" and "odd-q" offset coordinate systems mentioned here: https://www.redblobgames.com/grids/hexagons/#coordinates-offset

ColdAnkles commented 2 months ago

oh barf, I hate that Do we have any reason to keep it that way? (and thanks and kudos for the really clear and helpful diagram)

thelsing commented 2 months ago

I think the way it is quite logical.

Baaaaaz commented 2 months ago

oh barf, I hate that Do we have any reason to keep it that way? (and thanks and kudos for the really clear and helpful diagram)

No worries about the diagram - I just expanded on your better diagram idea! 👍

Maybe a separate FR to introduce a whole new (optional) alternative to offset hex grid coordinate referencing, but it is possible to translate between offset and axial coordinates: https://www.redblobgames.com/grids/hexagons/#conversions-offset

ColdAnkles commented 2 months ago

The problem now is that footprints are defined as a number of offsets from a reference point - the standard "Large" size token is comprised of the cells (0,0), (1,0), (0,1) which works A-OK when the reference point is also (0,0) since adding offsets to that results in identical set. But when the reference point is (-1,0) you end up with the set (-1,0), (0,0), (-1,1) instead of (-1,0), (0,1), (-1,1) - two entirely different shapes. I have no idea how it was working previously and why it's different now, since I'm using the same list of offsets as were originally used. Is it worth telling the getOccupiedCells function what grid you're on and attempting to handle the offset there, or something else?

bubblobill commented 2 months ago

If you read the redblob stuff about coordinate systems you will see that there are much better systems than the odd-r/q crap we went with.

Since I have been advocating we upgrade to cubic ever since I read about it I think this is a great opportunity to implement it. You wouldn't need a new PR to change the hard-coded xml to fit a sensible system, and adding a third axis will make future stuff much easier. Calculating radii and rings and other shapes programatically is much easier with a decent coordinate system. He even provides code for conversion between systems so a tiny bit of infill code can keep things running that don't rely on it. You can do all your operations on sensible stuff and just spit out the ugly for every other function to use.

bubblobill commented 2 months ago

oh barf, I hate that

Do we have any reason to keep it that way?

(and thanks and kudos for the really clear and helpful diagram)

No reason. Currently every size is just a bigger circle. You could throw out the existing stuff and generate them on startup in a couple of ms.

bubblobill commented 2 months ago

I think the way it is quite logical.

Just not optimal.

thelsing commented 2 months ago

I'm all for using a proper coordinate system (y-up) instead of the pixel-based (0,0) is top left. But this should be done everywhere and not just in a small part like tokenfootprint.

bubblobill commented 2 months ago

I'm all for using a proper coordinate system (y-up) instead of the pixel-based (0,0) is top left. But this should be done everywhere and not just in a small part like tokenfootprint.

Gotta start somewhere, and nobody wants to rewrite everything all at once.

bubblobill commented 2 months ago

So... Mr Ankles. You wish to grant supreme power to users to determine shapes. Now, as we all know by now, with great power comes much irresponsibility. One wonders what Mr @kwvanderlinde will want to know about things like this

The shape is not conveniently convex.
It has no evident centre other than the point all cells are relative to.
It wants to have eyeballs in the blue hex.
It has a glow in the dark tail.

I propose footprints contain the following data;

The cell from which vision is determined.
The cell from which light is calculated.
A point (not cell) on which rotation occurs, assumed to be the origin cell centre unless defined.

Just saying, this GUI is getting out-of-hand.

ColdAnkles commented 2 months ago

I propose footprints contain the following data;

1. The cell from which vision is determined.

2. The cell from which light is calculated.

3. A point (not cell) on which rotation occurs, assumed to be the origin cell centre unless defined.

I did some quick tests - looks like they originate from the point at the centre of the relative x/y rectangle bounding box.

Fundamentally I agree, but unsure if I want to get into extending footprints with that functionality at this point.

At any rate, I solved my offset grid issues and will open a new issue proposing changes to hex grid coordinate systems.

bubblobill commented 1 month ago

Minor annoyance - you cannot set the canonical name for a footprint, only the localised name. This is fine for people doing it for themselves, but annoying for wanting to edit the existing cases for broader use. Going to have to replace it with a new one which is extra fiddling.

ColdAnkles commented 1 month ago

That's something of a consequence of my choice to use the canonical name as the key for the map as well - the only way to do it would be to remove the existing named footprint and replace with an almost identical footprint.

bubblobill commented 1 month ago

Yup, just extra fiddling. GUI is nearly there (old pic). I have it on your fork so I am wary of pushing it until it is working otherwise you will need to exclude it from your commits.

bubblobill commented 1 month ago

Footprint functions:

People are going to ask for "Isometric" so you should include it, even if it only returns square.
You use mod in your offset translator calculations. Unwise as there is always the potential for a negative number to screw things up. Probably better to use bitwise And. (I know it's copy pasta, but that doesn't mean the original can't be improved on).

ColdAnkles commented 1 month ago

Some Feedback: If you delete the only footprint marked as a default, you start getting errors -> set the first footprint in the list to be default if no defaults exist? There's something I don't understand happening with the naming of footprints and the appending of underscores to them and this causing other footprints to disappear. Its not immediately clear how the cell selection works; I worked out it was Select A, then B sets cells on A->B path as footprint - maybe use blank button to swap between path select and single cell select? Cancel button didn't work.

bubblobill commented 1 month ago

Some Feedback: If you delete the only footprint marked as a default, you start getting errors -> set the first footprint in the list to be default if no defaults exist?

It is already supposed to do that

There's something I don't understand happening with the naming of footprints and the appending of underscores

Supposed to ensure unique names. Been redone a few times and I got distracted so it's probably between two different implementations

Cancel button didn't work.

Not sure if it's connected yet, but escape works.

Functionality is supposed to be:

Typing in the text field: Changes the name of the currently selected footprint
Add: Duplicates the current footprint with an amended name
Revert: Goes back to the original footprint if it exists. Not intended to work on new footprints but it might.
Clicking: Long story Firstly. The editing panel has gone through many iterations, in it's present form it accepts three arguments, a grid type name (nullable), a footprint, and a cell set (also nullable)(overrides getOccupiedCells()).
creates an instance of each type of grid
creates a new zone and zone renderer
switches between grids according to passed arguments
paints cell highlights for each of the footprint's cell set
paints a waypoint marker on the origin cell that everything is relative to
creates a ZoneWalker

The clicking:

mouselistener gets cell under mouse from the zone renderer, sends it to modifyFootprintCells
any clicks on cell(0,0) are rejected
any other cell is then added to or removed from the current cell set
if removed, the zone walker is reset to the origin point only
when added it is also added as a waypoint to the zone walker and the walker's cell path is added to the cell set. This started as an attempt to ensure that there are no isolated cells but it doesn't work. Basically I'm tinkering with ways to make sure we end up with a monolithic geometry with no holes.

bubblobill commented 1 month ago

Surprised you didn't notice the abusable scale setting

RPTools / maptool