Skip to content

Script entities #1320

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

Open
wants to merge 3 commits into
base: protocol_changes
Choose a base branch
from

Conversation

HifiExperiments
Copy link
Member

@HifiExperiments HifiExperiments commented Feb 24, 2025

adds a new Entity type, Script, with two properties: scriptURL and enabled. like material entities, script entities apply to their parents. if no parentID is set (or enabled is false), scriptURL won't run (but the existing script property, which applies to the entity itself, will).

one advantage of this approach is that each script can have its own separate user data/parameters. this is implemented as a new entity script method, updateUserData, which is called when userData changes

for a demo, run this script: https://gist.github.com/HifiExperiments/9da7d78021c4d3b08c7ca72ed03d846d/raw/f2a1228e5584167b0f3bfcfa9e16f8268158fd28/ScriptEntitiesTest.js

it creates a box with two child script entities, one which modifies the box's position, and one which modifies its dimensions. the dimension one is disabled at the start, but if you enable it in create, you can see that both will change at the same time!

thank you to @AleziaKurdis for the great new icons

@HifiExperiments HifiExperiments added documentation Improvements or additions to documentation enhancement New feature or request needs CR This pull request needs to be code reviewed needs QA This pull request needs to be tested server If used for a Pull Request, server packages are going to be built. protocol change labels Feb 24, 2025
@JulianGro
Copy link
Member

Without looking at your current code:
Maybe we should abstract this system in the Create app? Like, add a button to the Script tab of each Entities properties, which allows adding a second script (a script entity abstracted to appear the same as the initial entity script). Maybe even a button link (parent) script entities, which are missing a parent.
We could put the script entitiy's UUID and name into the tab, so script developers can tell that those scripts are separate entities.
Create app could also collapse those in the entity list, similar to the tree view.

I would just like to avoid the relative mess we currently have with material entities. Those are extremely powerful, but also really clunky and manual in their use.

@HifiExperiments
Copy link
Member Author

I’d like to save that for another PR, but yeah that’s exactly the idea, to be able to treat them as components of their parent entity in the UI, regardless of this internal implementation

I think we could do something similar for material entities to make them easier to use, so you can add them directly from the parent instead of having to switch between entities, but that’s all just UX on top

@AleziaKurdis
Copy link
Contributor

I agree, great features to add to Create app...
... but can be taken aside of this PR, since it is all app. side.

Add a Material Entities (from any entities)
Add a Script entities (from any entities)

Why not add any type entities as a child? (just an idea, any thought?)

also i was considering to add:
Copy color from Skybox (on the Ambient Light)

@HifiExperiments
Copy link
Member Author

for script entities, I think it would be good if on the script tab, all entities had a little list view of all the children script entities, with a button to take you to each of them and a button to add a new one.

similarly for material entities, not sure where to put it, but a list of all children material entities, with buttons to go to them, and a button to add a new one

yeah “add any entity type as a child” could also be useful! anything to save a few clicks. maybe a button next to parentID to go to the parent?

“Copy color from Skybox” also sounds great 😁

@AleziaKurdis
Copy link
Contributor

AleziaKurdis commented Feb 26, 2025

Here how I would plan to make this in create app:

  • Have a new tab "Child entities" on every entity property.

  • In that tab, have a list of child entities, with the name, the type, the hostType color, and a way to navigate to those entities properties. Doesn't render if multiple selection.

  • In that tab, have a "create a new child entity" feature which call the create with the parent ID to set. This feature could work for multiple selection. (Have a warning if applied to a multiple selection.)

  • Have navigation button to the parent next the parentID.

@HifiExperiments
Copy link
Member Author

@AleziaKurdis that sounds great! I think the button next to parentID which takes you to the parent would also be really useful

@ksuprynowicz
Copy link
Member

One thing that I think could be really useful, would be a field with named references to the entities that script will use.
Often scripts need to modify more than only the parent entity, and UUIDs change between script instances.
Could there be a entityReferences field, containing string-to-UUID map for entity references the script will need?
If we have such thing, then it would be really simple to spawn complex prefabs of entities with scripts from regular JSON file. The only thing we would need to add upon JSON import process is updating the references.
This would allow having a prefab repository that would be easily importable, even allowing multiple instances of the same prefab in a world.

@HifiExperiments
Copy link
Member Author

that’s a cool idea! you could use tags/names for this now but I see how that would be easier. maybe we can leave it for a separate PR?

if people are happy with the general idea here, then this is ready for review/testing from my end

@ksuprynowicz
Copy link
Member

that’s a cool idea! you could use tags/names for this now but I see how that would be easier. maybe we can leave it for a separate PR?

A big problem with tags and names is that they don't work if you spawn several instances of a given prefab in a single world. A standarized way of spawning prefabs would be very important for ease of creation.
I totally agree that it should be a separate PR though.

@HifiExperiments HifiExperiments added server If used for a Pull Request, server packages are going to be built. and removed server If used for a Pull Request, server packages are going to be built. labels Mar 30, 2025
Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

Everything looks good :)

@ksuprynowicz ksuprynowicz added CR approved This pull request has been successfully code reviewed and removed needs CR This pull request needs to be code reviewed labels Apr 5, 2025
@ksuprynowicz
Copy link
Member

I just tested it and entity server crashes very often with message:

terminate called after throwing an instance of 'std::bad_function_call'
  what():  bad_function_call
Signal: SIGABRT (Aborted)

Crash happens mostly on parenting script entity with a running script to another entity.
Script I used:

(function () {
    var clicked = false;
    this.clickDownOnEntity = function (entityID, mouseEvent) {
        if (clicked){
            //Entities.editEntity(entityID, { color: { red: 0, green: 255, blue: 255} });
            print("Clicked 1");
            clicked = false;
        } else {
            //Entities.editEntity(entityID, { color: { red: 255, green: 255, blue: 0} });
            print("Clicked 0");
            clicked = true;
        }
    };
    this.update = function (deltaTime) {
        print("update");
    }
})

Backtrace doesn't seem helpful - seems like a freshly started thread:

__pthread_kill_implementation 0x00007ffff429e95c
__pthread_kill_internal 0x00007ffff429e9ff
__GI_raise 0x00007ffff4249cc2
__GI_abort 0x00007ffff42324ac
<unknown> 0x00007ffff44a1a3d
<unknown> 0x00007ffff44b344a
std::terminate() 0x00007ffff44a15e9
qTerminate() 0x00007ffff5c9134d
<unknown> 0x00007ffff5c93308
start_thread 0x00007ffff429cb7b
__GI___clone3 0x00007ffff431a7b8

I'm attaching backtrace for all other threads as text file, because it's big.
I can also provide archive of a world where it happens.
bt_all_threads.txt

@ksuprynowicz
Copy link
Member

I also see that there's only one script URL field there. Do these support client side scripts only?

@HifiExperiments
Copy link
Member Author

thank you for testing! I will take a look. it is currently just for client side scripts, in the future we could add a toggle for server vs. client

@QEU-B-458
Copy link

cant wait till this is in interface it looks fun

@HifiExperiments
Copy link
Member Author

@ksuprynowicz crashes fixed and icons updated!

@HifiExperiments HifiExperiments added server If used for a Pull Request, server packages are going to be built. and removed server If used for a Pull Request, server packages are going to be built. labels Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CR approved This pull request has been successfully code reviewed documentation Improvements or additions to documentation enhancement New feature or request needs QA This pull request needs to be tested protocol change server If used for a Pull Request, server packages are going to be built.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants