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

Expose obj image offset and num images to plugins #17567

Conversation

nparshook
Copy link
Contributor

This PR makes two changes.

  1. Expose the base image offset and number of images for an Object via the plugin API.
  2. To support the above I've refactored the image allocation/deallocation into ImageTable.

For testing I opened up a park with all objects available and also wrote a toy plugin to check the images.

@nparshook nparshook force-pushed the expose-object-image-offset-and-count-in-plugin-api branch from 3ea73a6 to 674727e Compare July 18, 2022 18:54
@IntelOrca
Copy link
Contributor

I have a WIP branch where I have refactored the image loading code of objects. In this branch, I created two methods in Object.h: LoadImages() and UnloadImages().

Would it be possible for you to change _baseImageId = GetImageTable().AllocateImages(); to _baseImageId = LoadImages();?
And GetImageTable().FreeImages(); to UnloadImages.
These methods will need to be implemented in Object.h.
LoadImages should return an ImageIndex.

Also, you might as well change occurances of image offset to base image id for consistency.

As for exposing the base image index in plugins... I kind of get the rationale. But from what I understand in the discord chat, what you actually need are specific image IDs (footpath preview). In which case it would be better to expose those fields, so that plugins do not rely on the image order of the object as much.

Also, not all objects contain images, so it is somewhat specific to only a certain group of object types. Maybe create another interface that only objects containing images implement in openrct2.d.ts?

@nparshook
Copy link
Contributor Author

nparshook commented Jul 18, 2022

  1. If I understand correctly you would like me to change my ImageTable::Allocate/FreeImages functions into Object::Load/UnloadImages?
  2. Could you clarify change occurances of image offset to base image id for consistency, basically where are you talking about?
  3. I agree with exposing just specific fields. However, there is definitely more flexibility with exposing these fields on Object. I think it also makes for a cleaner interface. All objects have 0 or more images, and the preview image is always an Object's first image. Thoughts?
  4. Maybe instead of having a separate interface, I make the type an optional, readonly baseImageId: number|null?

@nparshook
Copy link
Contributor Author

Another question. What is the preferred style for addressing feedback in terms of commits? Do you like PRs to be squashed into a single commit or do you like multiple commits per PR?

@IntelOrca
Copy link
Contributor

IntelOrca commented Jul 18, 2022

  1. yes
  2. Change GetBaseImageOffset to GetBaseImageId, _image_offset to _baseImageId etc.

All objects have 0 or more images

Obviously, but some object types can not have images. And the preview image being the first one is not always going to be the case. Some object types have multiple preview images. But this point can be skipped r.e. point 4.

  1. Why are you not keen on seperate interfaces? What about just a new interface that implements LoadedObject called LoadedImageObject?

@nparshook
Copy link
Contributor Author

Mostly because I'm new to typescript and wasn't sure about how interfaces really worked (looks like ducktyping). Hence a little hesitant to add another. I'm going to do some more reading tonight and I'll push a commit tomorrow morning addressing your feedback.

Thanks for the help!

@nparshook nparshook force-pushed the expose-object-image-offset-and-count-in-plugin-api branch 2 times, most recently from 46985b8 to 5311520 Compare July 19, 2022 11:32
@nparshook nparshook marked this pull request as ready for review July 21, 2022 20:40
@IntelOrca
Copy link
Contributor

Music objects can now also contain images, otherwise looks fine.

@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as off-topic.

@github-actions github-actions bot closed this Apr 4, 2023
@janisozaur janisozaur reopened this Apr 4, 2023
@github-actions github-actions bot removed the stale-pr label Apr 5, 2023
@IntelOrca IntelOrca force-pushed the expose-object-image-offset-and-count-in-plugin-api branch from 5311520 to aaec560 Compare April 10, 2023 22:46
@IntelOrca IntelOrca force-pushed the expose-object-image-offset-and-count-in-plugin-api branch from aaec560 to d1be706 Compare April 10, 2023 23:06
@duncanspumpkin duncanspumpkin enabled auto-merge (squash) April 13, 2023 18:46
@duncanspumpkin duncanspumpkin merged commit ef35dfa into OpenRCT2:develop Apr 13, 2023
21 checks passed
@tupaschoal tupaschoal added this to the v0.4.5 milestone Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants