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

Replacement Vintage Story plugin #460

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

spooq
Copy link

@spooq spooq commented Jan 1, 2024

Can't figure out the animation not working right on import. Saving and reopening the file works though

var autosettings = [];
var namespace = {}

BBPlugin.register('vintagestory', {
Copy link
Owner

Choose a reason for hiding this comment

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

The PR title makes it seem like this is an update to the existing Vintage Story plugin. But this is a completely new file, with a different ID. Also, the old plugin source was removed, but without any changes to the meta data.

Copy link
Author

Choose a reason for hiding this comment

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

PR title updated, metadata updated

name: 'Vintage Story game folder',
description: 'Set this to the install path',
category: 'defaults',
value: process.env.APPDATA.replaceAll('\\', '/') + '/Vintagestory',
Copy link
Owner

Choose a reason for hiding this comment

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

This plugin is tagged to work both on the desktop and the web app. Here, you are trying to access data that would only be available in the desktop app, which causes an error in the web app.

Copy link
Author

Choose a reason for hiding this comment

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

tagged with desktop in plugin and metadata until I figure out how to switch setting_gamepath on/off based on variant

Copy link
Author

Choose a reason for hiding this comment

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

only set the default if isApp and Windows

.gitignore Outdated
@@ -1,4 +0,0 @@
node_modules/
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this change from the PR

Copy link
Author

Choose a reason for hiding this comment

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

removed


BBPlugin.register('vintagestory', {
title: "Vintage Story",
author: "Malik12tree,Crabb",
Copy link
Owner

Choose a reason for hiding this comment

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

Please separate different authors by a comma, followed by a space.

Copy link
Author

Choose a reason for hiding this comment

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

fixed

icon: "park",
description: "Export and import of Vintage Story shapes",
tags: ["Vintage Story"],
version: "1.0.2",
Copy link
Owner

Choose a reason for hiding this comment

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

The meta data was changed here, but the changes are not reflected in plugins.json.

Copy link
Author

Choose a reason for hiding this comment

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

metadata updated

image_editor: false,
integer_size: false,
java_face_properties: true,
locators: true,
Copy link
Owner

Choose a reason for hiding this comment

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

Locators are enabled, but not handled. Is this on accident?

Copy link
Author

Choose a reason for hiding this comment

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

turned off

icon: 'park',
image_editor: false,
integer_size: false,
java_face_properties: true,
Copy link
Owner

Choose a reason for hiding this comment

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

These are enabled, but none of the properties are actually used (shade, cullface, tint).

Copy link
Author

Choose a reason for hiding this comment

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

turned off, thought I needed these for the java texture window but appears not to be required

@spooq spooq changed the title Update Vintage Story support Replacement Vintage Story plugin Jan 21, 2024
update plugins.json
remove locators tag
remove java face tag
restore .gitignore
tag for desktop only until I find out how to switch features per variant
@JannisX11
Copy link
Owner

JannisX11 commented May 20, 2024

Sorry this hasn't been looked at in a while.
@Malik12tree Have you taken a look at this? What are your thoughts?

@Malik12tree
Copy link
Contributor

Looks perfect to me!
However, I'm concerned about the change of the plugin id, would that cause an error for the people who have the old plugin installed?
@spooq My only suggestion would be only to include VintageStory's icon as the plugin's icon (of course if it doesn't violate VintageStory's copyrights). But for now, the merge is applicable.

@Malik12tree
Copy link
Contributor

Also, a members.yml should be added welcoming you @spooq ! :)

maintainers:
  - Malik12tree
  - spooq

@JannisX11
Copy link
Owner

Yup, the ID change would mean that users would no longer have the old plugin and would have to manually install the new one. Up to you @spooq, you can also just re-use the the current ID, it just needs to be consistently changed in all files and paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants