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

Add World scripting features #3859

Merged
merged 21 commits into from
Jan 19, 2024
Merged

Conversation

dogboydog
Copy link
Contributor

@dogboydog dogboydog commented Dec 19, 2023

Fix #3539

Add UnloadAllWorlds action, exposed in World > Unload World when two or more worlds are loaded.

Added to scripting:

tiled.worldsChanged : Signal<void>;
tiled.worldLoaded : Signal<string>;
tiled.worldReloaded : Signal<string>;
tiled.worldUnloaded : Signal<string>;  
tiled.worldSaved : Signal<string>;
tiled.worlds : World[]; 

class WorldMapEntry {
  fileName : string;
  rect : rect;
} 

class WorldPattern {
  regExp: RegExp;
  multiplierX: number;
  multiplierY: number;
  offset: point;
  mapSize: size;
}

class World { 
  readonly maps : WorldMapEntry[];
  readonly patterns : WorldPattern[];
  allMaps() : WorldMapEntry[];
  mapsInRect(rect : rect) : WorldMapEntry[];
  containsMap(fileName : string) : boolean;
  setMapRect(fileName: string, rect : rect): void;
  setMapPos(map: TileMap, x: number, y: number): void;
  addMap(fileName: string, rect: rect): void;
  addMap(map: TileMap, x: number, y: number): void;
  removeMap(fileName: string): void;
  removeMap(map: TileMap): void;
  save(): boolean;
}

TODO

  • Update version number for @since in new entries in tiled.d.ts
  • Fix crash on tiled.worlds. I think the WorldDocument I'm creating is getting freed when worlds() returns in scriptmodule.cpp, but not sure where to save the references or if I'm right about the problem
  • adding/removing maps from worlds
  • Test and provide examples in below gist of methods on individual worlds returned from tiled.worlds

Test script
https://gist.github.com/dogboydog/42edb0886989467ec3671acdce4e749f#file-worldscripting

… or more worlds are loaded

expose world related signals to scripting
expose loading, unloading, accessing worlds to scripting
@dogboydog dogboydog marked this pull request as draft December 19, 2023 02:47
Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments, but to be continued next week. :-)

src/tiled/editableworld.h Outdated Show resolved Hide resolved
src/tiled/editableworld.h Outdated Show resolved Hide resolved
bjorn and others added 7 commits January 16, 2024 16:19
This also makes sure the UI adjusts immediately to the changes, since
the WorldManager::worldsChanged signal is emitted.

Also added some checks on the function parameters.
At least as it stands, this class is only relevant in the editor. The
world load and save functions, previously part of WorldManager, are now
part of World.
This way we don't need the ScriptWorld class.

Now a worlds can in theory get custom properties assigned to it. In
practice, you can only do this through the scripting API for now, and
these properties currently do not get saved (though that should be easy
to add).
Now QVector<WorldMapEntry> and QVector<WorldPattern> can be exposed to
JS directly. This only works with Qt 6 (tested with 5.15, 6.5 and 6.6),
but I don't think it's worth supporting these properties for Qt 5
builds.

Also removed World.displayName, since I think this was just a
convenience member which isn't very useful to add to the scripting API.
In the end it was just returning the file name part of the path returned
by World.fileName.

Improved the documentation in a few places.
@dogboydog dogboydog marked this pull request as ready for review January 17, 2024 16:00
@dogboydog dogboydog changed the title [DRAFT] Add World scripting features Add World scripting features Jan 17, 2024
@bjorn
Copy link
Member

bjorn commented Jan 17, 2024

Feedback to address:

  • It is not possible to add maps to worlds that also use patterns. This need to be either supported or an error needs to be reported when trying to do this.
  • Add overloads for addMap that take a Tiled::EditableMap*, with only a position (since this one can calculate the size automatically).

@eishiya
Copy link
Contributor

eishiya commented Jan 17, 2024

  • Add overloads for addMap that take a Tiled::EditableMap*, with only a position (since this one can calculate the size automatically).

If adding maps can be done with a TileMap, then I think removeMap(TileMap) should be available too. That one should be simpler since it just needs to check whether the TileMap has a fileName and use that.

No need for slots when they just emit the signal with the same
parameter.
@bjorn
Copy link
Member

bjorn commented Jan 17, 2024

Another small thing: I think there should be a menu separator above the "Unload All Worlds" action.

@dogboydog
Copy link
Contributor Author

I added addMap and removeMap which take EditableMap instances. I'm wondering if we should abstract away any references to mapIndex in any of these world scripting APIs and just have the user specify either an EditableMap or a file name. Thanks for your help with this.

@bjorn
Copy link
Member

bjorn commented Jan 18, 2024

I'm wondering if we should abstract away any references to mapIndex in any of these world scripting APIs and just have the user specify either an EditableMap or a file name.

Yeah, I'll have a go at removing all the index-based API. If necessary one can compute the index by iterating world.maps anyway. Otherwise for completeness sake we'd have to provide an index, filename and map version of each function...

Also fixed the size of the map to be calculated using the MapRenderer
when adding a map at a position.

Removed duplicate documentation for World.mapIndex.
@eishiya
Copy link
Contributor

eishiya commented Jan 18, 2024

Is using a QPoint instead of just x, y necessary? I feel like addMap(map, x, y) would be more convenient than addMap(map, Qt.point(x, y)).

Also implemented saving of patterns, making a best effort to keep the
pattern definition minimal by checking for default values.

This should allow hybrid worlds (with maps and patterns) to at least
have their maps modified through the scripting API. The world tool
still disallow any edits, for worlds that also use patterns.
Based on feedback from @eishiya, this is probably more convenient.
@bjorn
Copy link
Member

bjorn commented Jan 18, 2024

Alright, I think it's done and ready for merge. Would you give it a final round of review and testing, @dogboydog?

@dogboydog
Copy link
Contributor Author

Thanks, I just gave it a try. I was able to run my example script and add and remove maps, call mapsInRect, containsMap, setMapPos. The signals seem to be firing as expected. It seems good to me.

Removed worldLoaded, worldReloaded, worldUnloaded and worldSaved.

Made sure the following signals are also emitted for worlds:

* assetOpened (replaces worldLoaded)
* assetAboutToBeSaved
* assetSaved (replaces worldSaved)

Moved World.save() up to Asset.save(). Its behavior changed slightly, in
that errors are now reported in a modal dialog, due to reusing the
existing saving code.

There is no replacement for worldReloaded and worldUnloaded, but
hopefully worldsChanged suffices for most use-cases.

Fixed tiled.worldsChanged to only fire when the editor is loaded, not on
the CLI.
@bjorn
Copy link
Member

bjorn commented Jan 19, 2024

@dogboydog I've decided to still push a large change and remove most of the world-specific signals, readding some of them by emitting assetOpened, assetAboutToBeSaved and assetSaved also for worlds. I wasn't convinced the existing world-specific signals would be useful enough, and it could be hard to keep API compatibility when we change the way worlds are handled in the future.

@bjorn bjorn merged commit fb2ccba into mapeditor:master Jan 19, 2024
15 of 16 checks passed
@dogboydog dogboydog deleted the 3539_script_worlds branch May 14, 2024 14:30
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.

Scripting: Add API for Worlds
3 participants