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

Services can't be used as partition targets on current git build #84

Closed
Kampfkarren opened this issue Aug 17, 2018 · 8 comments
Closed

Comments

@Kampfkarren
Copy link
Member

On the release version, this isn't an issue, but cloning and building rojo yourself doesn't let you use a service as a partition target.

image

Unsure if this is already known. If it is, take this as a tracking issue I suppose.

@LPGhatguy
Copy link
Member

This is a good reminder!

I was still mulling through the best solution to this for the rewrite, and there are a couple options if I remember right. I should've written them down:

  1. Use wildcard instance class names that are created using either GetService or folders if they exist, or reuse instances if they already exist. This is what 0.4.x does and is the root cause of Deleting init.model.json does not revert parent to Folder #76.
  2. Make Rojo aware of all services and service-like things, and either:
    1. Make the server return instances with class names that match
    2. Use a limited subset of the wildcard system, but only for 'service-like' objects
  3. Make users annotate objects more closely, which makes Support using game as a partition target #26 really annoying for consumers, but makes Rojo a lot simpler.

@Kampfkarren
Copy link
Member Author

Isn't the behavior not being that of bullet point 1. also why everything gets purged on sync?

As for #2, you should always be targeting a service anyway. Should be easy to tell Rojo that the first ancestor on a partition target will always be a service. (i.e. if given ReplicatedStorage.Foo.Bar, Rojo should automatically know ReplicatedStorage is a service since it's the first ancestor). This brings up issues with people renaming services from their original class name, but you could also enforce the first ancestor being a service class name rather than its real name.

@LPGhatguy
Copy link
Member

Consider StarterPlayer.StarterPlayerScripts, which is a service-like object that you want to preserve. That's sort of the reason why 0.4.x went with the route from 1.

Another decision that I made in 0.4.x was that reaching the root of a partition (navigating the actual path field) and filling out the partition are treated differently. This sort of fell out of the same reasoning as bullet 1.

@LPGhatguy
Copy link
Member

I just had an insane epiphany after replying here, I'll push a commit to master in a little bit.

It's the solution!

@Kampfkarren
Copy link
Member Author

Kampfkarren commented Aug 20, 2018

StarterPlayer/CharacterScripts is an ugly edge case. Looking forward to seeing your fix.

@LPGhatguy
Copy link
Member

Okay, it ended up being pretty involved, but the rough stuff will be in the epiphany branch.

The major thing to note is the replacement of the existing partition config format in favor of something hierarchical:

https://github.com/LPGhatguy/rojo/blob/1a2ee07d37e9862d64084097d7dbb2b5a07ea71e/test-projects/foo.json

This lays the way for hierarchical settings along the lines of preserveUnknownObjects on a per-object level. It solves the overlapping partitions problem, the service problem (Rojo can optionally infer types if they're omitted, but probably won't at first), a few problems with two-way sync, and ends up producing something that's probably a lot more intuitive.

The config format I'm proposing is kind of strange. It mixes properties and children into the table to avoid creating insane nesting depth (and redundancy, specifying $className all the time). Other alternatives include switching to XML or dealing with the extra children keys, which could be alleviated by having a GUI config editor.

@Kampfkarren
Copy link
Member Author

Switching to XML or a DSL seems like a way to make that syntax better. I like the idea though!

@LPGhatguy
Copy link
Member

Going to close this right now, since the master branch has been refactored.

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

No branches or pull requests

2 participants