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

[Tag] for CollectionService tags #36

Open
Anaminus opened this issue Aug 30, 2021 · 11 comments
Open

[Tag] for CollectionService tags #36

Anaminus opened this issue Aug 30, 2021 · 11 comments
Labels
kind: enhancement New feature or request status: approved Enhancements/changes that will be made

Comments

@Anaminus
Copy link
Contributor

Add a [Tags] symbol to support construction of instances with tags. Similar to [Children], except the elements are just strings:

-- Single tag construction.
New "Folder" {
	[Tags] = "Foo",
}

-- Multiple tag construction.
New "Folder" {
	[Tags] = {"Foo", "Bar"},
}

-- Nested tag construction.
New "Folder" {
	[Tags] = {"Foo", "Bar", {"Baz"}},
}

Support for states?

local tag = State("Foo")

local folder = New "Folder" {
	[Tags] = tag,
}

tag:set("Bar") -- Adds "Bar", removes "Foo".
local tagA = State("Foo")
local tagB = State("Foo")

local folder = New "Folder" {
	[Tags] = {tagA, tagB},
}

tagA:set("Bar") -- Adds "Bar", "Foo" still retained by tagB.
tagB:set("Bar") -- Adds "Bar", removes "Foo".
local tags = State({"Foo", "Bar"})

local folder = New "Folder" {
	[Tags] = tags,
}

tags:set({"Bar", "Baz"}) -- Removes "Foo", adds "Baz".
tags:set("Foo") -- Removes "Bar" and "Baz", adds "Foo".

This could make use of #10 under the hood if that is implemented. Alternatively, it could detect keys instead of values, but seems somewhat less convenient.

Another consideration is whether [Tags] represents all tags on the instance, or only the tags known by bound states.

local tags = State({"Foo", "Bar"})

local folder = New "Folder" {
	[Tags] = tags,
}

CollectionService:AddTag(folder, "Baz")
tags:set({"Bar"}) -- Removes "Foo", should this remove "Baz"?

[Children] has a similar consideration, so [Tags] should match whatever that does.

@Anaminus
Copy link
Contributor Author

Since tags are unordered like properties, a possible alternative API:

New "Folder" {
	[Tag "Foo"] = true,
	[Tag "Bar"] = true,
}

Though this eliminates the possibility of binding a set of tags. On the other hand, this would be a good solution if binding sets of tags were expressly unwanted.

@dphfox
Copy link
Owner

dphfox commented Aug 30, 2021

This is an interesting idea! I'm assuming this is talking about CollectionService tags?

@Anaminus Anaminus changed the title Support instance tags Support CollectionService tags Aug 31, 2021
@Dionysusnu
Copy link
Contributor

I would go for the first option, having a tags symbol like children. On a similar note, this should probably be added for attributes too?

@dphfox dphfox added the kind: enhancement New feature or request label Sep 8, 2021
@dphfox
Copy link
Owner

dphfox commented Sep 8, 2021

On a similar note, this should probably be added for attributes too?

Tracking that here: #1

@dphfox
Copy link
Owner

dphfox commented Feb 1, 2023

The design of this API will probably be guided by #1

@Aloroid
Copy link
Contributor

Aloroid commented Mar 6, 2023

Tags shouldn't be designed like Attributes. How would it even work in actual code? All keys need a value, so do we just have to set it to true in all cases because yes? Do we make the value a boolean which controls if it has that tag or not? It also looks a lot harder to guess what the value that we set it to means.

New "Frame" {
    [Tag "MyTag"] = true
}

This looks incredibly awkward to read imo. In comparison to the original proposal,

New "Frame" {
    [Tags] = {"MyTag"}
}

This looks way better in my eyes as we aren't pretending to store some magical value.
It also makes using states more pleasing on the eyes.
[Tag(tagName)] = true vs [Tags] = {tagName}

@dphfox
Copy link
Owner

dphfox commented Aug 22, 2023

I think we'll go for Tags syntax here, it seems more appropriate given there isn't really a pair of values. Thanks for the feedback.

@dphfox dphfox added status: approved Enhancements/changes that will be made and removed status: needs design Good idea but needs design work labels Aug 22, 2023
@Dionysusnu
Copy link
Contributor

I think we'll go for Tags syntax here, it seems more appropriate given there isn't really a pair of values. Thanks for the feedback.

Will any excess tags be removed automatically? Eg if Hydrateing an instance that already has a generic Weapon tag, and wanting to add either Gun or Knife depending on state? What if Tags is not specified, will that remove excess tags too? That creates the confusing option where adding something in the code actually ends up removing more things at runtime.
I think I've changed my earlier opinion and prefer the option of treating them like [Tag "A"] = bool

@Anaminus
Copy link
Contributor Author

Regarding extraneous tags, Fusion would have to deliberately go out of its way to remove them, and that would be an unnecessary step from Fusion's PoV. To implement tag states, Fusion would just be making calls to AddTag and RemoveTag as needed to reconcile state changes. As long as a tag isn't involved in this state, then it would remain untouched.


Regarding the syntax, my original use-case only used tags as constants, so I don't really have a stake in either. But I can try to imagine scenarios that involve state objects. The Tags syntax seems like it would be a lot easier to use in general.

local itemRelatedTags = Value({}::{string})
local weaponRelatedTags = Value({}::{string})

local sword = Sword {
	[Tags] = {
		itemRelatedTags,
		weaponRelatedTags,
	},
}

itemRelatedTags:set({"Durability", "Countable"}) -- sword gains Durability and Countable.
weaponRelatedTags:set({"Durability", "Damaging"}) -- sword gains Damaging.
itemRelatedTags:set({}) -- sword loses Countable.
weaponRelatedTags:set({}) -- sword loses Damaging.

Meanwhile, the same behavior with the Tag syntax would be a lot more verbose:

local itemRelatedTags = Value({}::{[string]:true})
local weaponRelatedTags = Value({}::{[string]:true})

local sword = Sword {
	[Tag "Durability"] = Computed(function(use)
		return use(itemRelatedTags.Durability) or use(weaponRelatedTags.Durability)
	end),
	[Tag "Countable"] = Computed(function(use)
		return use(itemRelatedTags.Countable)
	end),
	[Tag "Damaging"] = Computed(function(use)
		return use(weaponRelatedTags.Damaging)
	end),
}

itemRelatedTags:set({Durability=true, Countable=true})
weaponRelatedTags:set({Durability=true, Damaging=true})
itemRelatedTags:set({})
weaponRelatedTags:set({})

While awkward for this particular scenario, we're being much more deliberate about what tags are defined on the instance, which might be seen as a benefit in some situations.

I think it depends on the situation, and I'd wager that both syntaxes have their ideal use-cases. If I really had to pick a side, I'd be on team por-que-no-los-dos.

@dphfox
Copy link
Owner

dphfox commented Aug 23, 2023

Yeah this is the sort of thing I'm thinking about. We could do both perhaps, no reason not to.

This perhaps could inform the reactive tag APIs in #206

@dphfox
Copy link
Owner

dphfox commented Feb 5, 2024

Both? Both. Both. Both is good.

But we'll divvy it up a bit; if we're adding Tags, we should probably consider whether there are reasonable interpretations of Attributes, Properties, Events, etc. There's already some demand for bulk property setting over in #137, so I think that's a valuable discussion.

So what I'll do is I'll limit the scope of this particular change to Tag, and I'll broaden the scope of the other issue to encompass bulk setting as a general concept.

@dphfox dphfox changed the title Support CollectionService tags [Tag] for CollectionService tags Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement New feature or request status: approved Enhancements/changes that will be made
Projects
Status: To Do
Development

Successfully merging a pull request may close this issue.

4 participants