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

A function to return the dimension of an image from its assetID #949

Closed
Merudo opened this issue Nov 28, 2019 · 13 comments
Closed

A function to return the dimension of an image from its assetID #949

Merudo opened this issue Nov 28, 2019 · 13 comments
Assignees
Labels
feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.

Comments

@Merudo
Copy link
Member

Merudo commented Nov 28, 2019

Is your feature request related to a problem? Please describe.
It is to my knowledge impossible to get the dimensions of an image from it's asset ID. It makes it difficult to deal with images properly, for example to display an image in a frame of proper dimensions.

Describe the solution you'd like
A function, maybe called getAssetProperties, to return a json holding the properties of an asset. Right now only images are assets, so the json could be something like:

["type":"image", "width":300, "height", 200]

Merudo added a commit to Merudo/maptool that referenced this issue Nov 28, 2019
- Function takes the asset id as parameter
- Function returns "type", "id", "name", "status", "width" "height".
- Close RPTools#949
@Phergus Phergus added the feature Adding functionality that adds value label Nov 28, 2019
@Phergus
Copy link
Contributor

Phergus commented Nov 28, 2019

Technically the getTokenNativeWidth/Height functions did this but the problem is that they depend on values set on the token and sometimes the values don't represent the actual image dimensions. Going straight to the asset for the info is the right way IMO.

Merudo added a commit to Merudo/maptool that referenced this issue Nov 28, 2019
- Function takes the asset id as parameter
- Function returns "type", "id", "name", "status", "width" "height".
- Close RPTools#949
@Azhrei
Copy link
Member

Azhrei commented Nov 28, 2019

I’m fine with this. But I’m thinking using the MIME type of the asset and splitting it into two fields would be easier for consumption.

I don’t know what other kinds of assets might exist in the future, but MIME types seem like a good place to start. I can see text assets needing a character encoding and SVG ones needing a default scale, but knowing which properties to expect will be driven by MIME type...

@Phergus
Copy link
Contributor

Phergus commented Nov 28, 2019

Using the MIME type would be a good idea. Too bad that the type is never set in Asset.java. The class has a field for it and a getter but no setter. It also has extension but that is only set to null.

It appears that the name is always set so putting the name in the JSON return might be nice.

@Azhrei
Copy link
Member

Azhrei commented Nov 28, 2019

IIRC, the AssetManager also keeps track of where the asset originally came from (repo, URL, etc) so maybe that would be good to include. I don't know that much else is tracked (like last-modified time stamp and such). It might be that the more detailed metadata has to be looked up in the Assetmanager instead of getting it from theAsset itself...

@Phergus
Copy link
Contributor

Phergus commented Nov 28, 2019

I don't see any info beyond filename - without extension - being captured by AssetManager. The .info file in the asset cache only has the filename and creation date in it.

Merudo added a commit to Merudo/maptool that referenced this issue Nov 28, 2019
- Function takes the asset id as parameter
- Function returns "type", "subtype", "id", "name", "status", "width" "height".
- Close RPTools#949
@Merudo
Copy link
Member Author

Merudo commented Nov 28, 2019

PR #950: I added the subtype (extension) to the properties returned.

@Merudo
Copy link
Member Author

Merudo commented Nov 29, 2019

Technically the getTokenNativeWidth/Height functions did this but the problem is that they depend on values set on the token and sometimes the values don't represent the actual image dimensions. Going straight to the asset for the info is the right way IMO.

Agreed, plus even fixed getTokenNativeWidth/Height wouldn't work very well for table images.

@Phergus
Copy link
Contributor

Phergus commented Nov 29, 2019

A good point as well.

@Phergus Phergus added documentation needed Missing, out-of-date or bad documentation macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) labels Nov 29, 2019
@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Nov 29, 2019
@Merudo
Copy link
Member Author

Merudo commented Nov 30, 2019

I noticed getting the properties from a table gives the wrong width and height if the asset isn't loaded yet - it returns the width and height of the "loading asset" image.

I suggest replacing the method "getImage" by "getImageAndWait", so the function waits until the image is fully loaded before returning its properties. Or, maybe the user could have the option to wait or not with an extra parameter?

@Phergus
Copy link
Contributor

Phergus commented Nov 30, 2019

It does return "transferring" in the status but that puts the onus of handling the delay on the macro coder. Is there a risk using getImageAndWait() that a macro could get hung up waiting for an image that never loads? The CountDownLatch does a timeout on it?

@Merudo
Copy link
Member Author

Merudo commented Nov 30, 2019

I don't know enough to tell if getImageAndWait can hang up a macro. I know the method is used at multiple times without issues.

A timeout could be added to the await method of CountDownLatch.

@Phergus
Copy link
Contributor

Phergus commented Nov 30, 2019

I'd rather not add a timeout to await. Switching to getImageAndWait is probably safe. If there is an issue it would show up in the other places it is used as well.

Go for it.

@Phergus Phergus removed tested This issue has been QA tested by someone other than the developer. documentation needed Missing, out-of-date or bad documentation labels Nov 30, 2019
Merudo added a commit to Merudo/maptool that referenced this issue Dec 1, 2019
- Fix getAssetProperties to return the correct width and height when the asset is not loaded into the cache.
- Close issue discussed in RPTools#949
@Phergus Phergus added the tested This issue has been QA tested by someone other than the developer. label Dec 2, 2019
@Phergus
Copy link
Contributor

Phergus commented Dec 2, 2019

Tested new change. No problems found.

@Phergus Phergus closed this as completed Dec 2, 2019
kwvanderlinde added a commit to kwvanderlinde/maptool that referenced this issue May 16, 2024
… images asynchronously

This is mostly a matter of patching ImageManager into the Swing ecosystem. And wrestling with the bizarreness known as
cell renderers...

[async] _Technically_ breaking change, but why supporting reporting TRANSFERING_IMAGE if we're going to block and wait for a non-transfering one anyways

See [RPTools#949](RPTools#949), but I disagree with the reasoning. Fight me.

[async] Max our marker size; not a correct change, but enough for POC non-blocking stuff; actually this related to previous commit as well where we only block so we can get accurate dimensions. Surely we can get dimensions without having to block. In my perfect world, our "image manager" is a source of truth for all things image, with metadata such as dimensions being serialized in the campaign for immediate lookup

[async] Add TODO notes about AssetPanel not really being about assets

[async] Non-blocking bar icons

[async] Non-blocking impersonation panel

[async] Non-blocking map asset for MapPropertiesDialog

[async] Update PointerTool's image observer

[async] FIXUP

[async] FIXUP Avoid ImageObserver

[async] Avoid non-chalant redundant image requests just for dimensions

[async] Now do token overlays (states/bars on the map)

[async] StateListRenderer now updates the JList when an image is resolved

[async] Resimplify TokenBarController to just notify the JList on update

[async] Initiative renderer also keeps track of the parent list now

[async] TokenPanelTreeCellRenderer now properly updates

[async] BarTokenOverlay and children now support observers

[async] Remove Token.getIcon() in favour of callers figuring out rendering concerns themselves; also remove other unused parts of Token

[async] FIXUP Avoid ImageObserver

[async] InitiativeListCellRenderer no longer blocks

[async] Add TODO note on getImageAndWait() inside of Token class

[async] Add asynchrony to image URL loading

This afffect chat, HTML 3 frames, and HTML 5 frames. Additionally includes a fix to AssetURLConnection to set up in
`connect()` - which gets called once - rather than `getInputStream()` - which can be called many times.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Adding functionality that adds value macro changes This issue adds or changes macro functions. Extra work is required (testing, wiki, code editor) tested This issue has been QA tested by someone other than the developer.
Projects
None yet
Development

No branches or pull requests

3 participants