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

DungeonManager by Kory, Warp fixes #434

Closed
wants to merge 37 commits into from

Conversation

jneb802
Copy link
Contributor

@jneb802 jneb802 commented Jul 17, 2024

Adding a new DungeonManager to Jotunn, allowing devs to add custom dungeon rooms to Valheim. Fixes and adjustments by Warp.

JotunnLib/Managers/DungeonManager.cs Show resolved Hide resolved
JotunnLib/Managers/DungeonManager.cs Outdated Show resolved Hide resolved
JotunnLib/Managers/DungeonManager.cs Outdated Show resolved Hide resolved
Comment on lines +238 to +239
// TODO - unsure if cache clearing is still necessary for resolving mocks of these assets
//ClearPrefabCache(typeof(Material));
Copy link
Member

Choose a reason for hiding this comment

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

it's fine to preemptively clear the Material cache. It's not an expensive operation unlike clearing the GameObject cache, because there are a lot more GameObjects than materials

JotunnLib/Managers/DungeonManager.cs Show resolved Hide resolved
Comment on lines 40 to 48
/// <summary>
/// Theme name of this room.
/// </summary>
public string ThemeName { get; private set; }

/// <summary>
/// <see cref="Room.Theme"/> of this room.
/// </summary>
public Room.Theme Theme { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

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

Like in the original PR, this should be changed to primarily use the string ThemeName and convert to the Room.Theme internally if needed (can be cached).

At the very least, Room.Theme Theme should be made internal and not exposed to the API, a dev shouldn't have to worry about when to choose what or even theme name changes. Take a look at https://github.com/Valheim-Modding/Jotunn/blob/23cc2fd84ec72a1f19d17da4b8ed2f2b561af9e7/JotunnLib/Configs/CraftingStations.cs and

set => craftingStation = CraftingStations.GetInternalName(value);
for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 5084fd2

if (roomConfig.Weight != null) room.m_weight = roomConfig.Weight.Value;
if (roomConfig.FaceCenter != null) room.m_faceCenter = roomConfig.FaceCenter.Value;
if (roomConfig.Perimeter != null) room.m_perimeter = roomConfig.Perimeter.Value;

Copy link
Member

Choose a reason for hiding this comment

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

From 366b7ab:

Suggested change
if (roomConfig.MusicPrefab != null) room.m_musicPrefab = Mock<MusicVolume>.Create(roomConfig.MusicPrefab);

A music prefab sounds nice to have, if MusicPrefab is a string the mock system can handle it. Haven't tested this tho. Maybe the outer FixReference can be set to true if MusicPrefab is set, for convince

@MSchmoecker
Copy link
Member

Included in #430

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.

2 participants