マルチ転送フィルターを設定可能にする #62

torikizi · 2024-12-19T07:38:51Z

ForwardingFilter に name と priority を追加
forwardingfilters を設定できるよう追加

This pull request introduces new functionality for configuring forwarding filters, including adding names and priorities, and deprecates the old forwarding filter method. Additionally, it updates the SoraSample.cs file to reflect these changes.

New Features:

Added the ability to configure multiple forwarding filters with new attributes such as name and priority. (CHANGES.md, SoraUnitySdkSamples/Assets/SoraSample.csR126-R194)

Code Updates:

Introduced a new ForwardingFilter class with attributes and a ConvertForwardingFilter method to handle the conversion of these filters. (SoraUnitySdkSamples/Assets/SoraSample.cs, SoraUnitySdkSamples/Assets/SoraSample.csR126-R194)
Deprecated the old forwarding filter configuration in favor of the new multiple forwarding filters configuration. (SoraUnitySdkSamples/Assets/SoraSample.cs, [1] [2]
Removed the old ForwardingFilterMetadata class and its usage from the OnClickStart method. (SoraUnitySdkSamples/Assets/SoraSample.cs, [1] [2]

torikizi · 2024-12-19T07:44:19Z

SoraUnitySdkSamples/Assets/SoraSample.cs

    public RuleList[] forwardingFilter;
    public bool enableForwardingFilterVersion = false;
    public string forwardingFilterVersion = "";
    public bool enableForwardingFilterMetadata = false;
    public string forwardingFilterMetadataMessage = "";

+


マルチ転送フィルター用に新しく項目を追加しました

torikizi · 2024-12-19T07:48:03Z

SoraUnitySdkSamples/Assets/SoraSample.cs


+                if (!string.IsNullOrEmpty(filter.action))


通常の転送フィルターでは enable を使って未設定にし、sora へ送信しないようにしていたが、マルチ転送フィルターは転送フィルターの項目が階層で一つおりたので設定しようとしない限りは送らない想定

melpon · 2024-12-20T14:06:55Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+    public bool enableForwardingFilterName = false;
+    public string forwardingFilterName;
+    public bool enableForwardingFilterPriority = false;
+    public int forwardingFilterPriority;


全体的に ForwardingFilters の設定と共通化して下さい

ForwardingFilter の設定と共通化しました。8521ea0

このあたりの設定も共通のクラスにできるはずです

torikizi · 2024-12-20T15:48:00Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                }
+
+                newFilter.Version = filter.version;
+                if (!string.IsNullOrEmpty(filter.metadata))


json への変換は他の項目は別の場所で行なっていますが、複数の転送フィルターを設定することを考えてこちらで実行するようにしました

melpon · 2024-12-21T00:06:18Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+    public bool enableForwardingFilterName = false;
+    public string forwardingFilterName;
+    public bool enableForwardingFilterPriority = false;
+    public int forwardingFilterPriority;


このあたりの設定も共通のクラスにできるはずです

melpon · 2024-12-21T00:07:09Z

SoraUnitySdkSamples/Assets/SoraSample.cs

    public RuleList[] forwardingFilter;
    public bool enableForwardingFilterVersion = false;
    public string forwardingFilterVersion = "";
    public bool enableForwardingFilterMetadata = false;
    public string forwardingFilterMetadataMessage = "";

+    [System.Serializable]
+    public class ForwardingFiltersItem


Rule クラスの命名ルールからすると単に ForwardingFilter で良さそう

melpon · 2024-12-21T00:08:53Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+
+            foreach (var filter in forwardingFilters)
+            {
+                var newFilter = new Sora.ForwardingFilter();


共通のクラスにすればこのあたりの処理は forwardingFilter に対する処理と共通化できるはず

melpon · 2024-12-21T00:13:31Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                newFilter.Version = filter.version;
+                if (!string.IsNullOrEmpty(filter.metadata))
+                {
+                    var ffm = new ForwardingFilterMetadata()


この ForwardingFilterMetadata がなんのために存在してるクラスなのか分からないので削除して良さそう

melpon · 2024-12-21T00:16:59Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                    newFilter.Priority.HasValue ||
+                    newFilter.Rules.Count > 0 ||
+                    newFilter.Version != null ||
+                    newFilter.Metadata != null)


仕様上、Action や Name、Priority に値が設定されてない場合でも ForwardingFilters は使えるはずなので、この分岐はダメそう

melpon · 2024-12-21T00:21:19Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                {
+                    newFilter.Name = filter.name;
+                }
+                if (filter.priority >= 0)


Unity に設定される int のデフォルト値が 0 で、priority に渡す値として 0 が有効な値である以上、ここで priority >= 0 のチェックする意味は無いです

melpon · 2024-12-21T00:24:54Z

SoraUnitySdkSamples/Assets/SoraSample.cs


+                if (filter.rules != null)
+                {
+                    foreach (var ruleSet in filter.rules)


filter.rules は RuleList 型であって ruleSet という名前は紛らわしいので別の名前にした方が良さそう

melpon · 2024-12-21T00:26:15Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                {
+                    foreach (var ruleSet in filter.rules)
+                    {
+                        if (ruleSet.data != null)


インデントが深くなるのでガード条件にした方が良さそう

if (ruleSet.data == null) { continue; } ...

melpon · 2024-12-21T00:28:28Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                                    ruleList.Add(newRule);
+                                }
+                            }
+                            if (ruleList.Count > 0)


空で送るテストすることを考えると、この条件は無くても良いです

melpon · 2024-12-21T00:29:28Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                            {
+                                if (!string.IsNullOrEmpty(rule.field) &&
+                                    !string.IsNullOrEmpty(rule.op) &&
+                                    rule.values != null)


ユーザーがインスペクタで要素数を指定して作っている以上、ここは空文字列が来ても設定して Sora に送るほうが素直で良いので、ここら辺の条件はいらないです

torikizi · 2024-12-23T15:53:12Z

共通のヘルパー関数を作ってスリムに作り直しました

melpon · 2024-12-24T05:13:25Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+    public static class ForwardingFilterHelper
+    {
+        [Serializable]
+        private class ForwardingFilterMetadata


そもそもこれって ForwardingFilter に渡すメタデータ用のクラスなので、string metadata の代わりに ForwardingFilterMetadata metadata にした方が良いです。

private class ForwardingFilterMetadata { public ForwardingFilterMetadata metadata; }

というようにした方が良いというご指摘だと理解したのですが、ここは JSON 用のプロパティのため、string にしておかないとうまくいかないようです

いや、ForwardingFilter クラスに定義してある string metadata を、string じゃなく ForwardingFilterMetadata にした方が良いよねって話です

失礼しました。勘違いしておりました。
以下のように修正します。

public ForwardingFilterMetadata metadata = new ForwardingFilterMetadata();

インスペクタで設定してもらう値だから初期値はいらないはず

melpon · 2024-12-24T05:14:54Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+            if (settings.enableVersion) filter.Version = settings.version;
+            if (settings.enableMetadata)
+            {
+                filter.Metadata = ForwardingFilterMetadataHelper.ConvertMetadataToJson(settings.metadata);


ここでしか ConvertMetadataToJson 関数を使ってないので、この関数に処理を分ける必要は無いです

if (forwardingFilter.enableMetadata) の中にまとめます

melpon · 2024-12-24T05:18:16Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+        }
+        public static Sora.ForwardingFilter CreateFilter(ForwardingFilterSettings settings)
+        {
+            if (settings == null) return null;


Create 関数で入力が null は前提条件として破綻してると思うので、この関数を呼び出す側がチェックした方が良いです。

forwardingfilter と forwardingfilters に enable フラグを追加してチェックするように修正します。

melpon · 2024-12-24T05:18:49Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+            if (settings == null) return null;
+
+            var filter = new Sora.ForwardingFilter();
+            ConfigureFilter(filter, settings);


これ１箇所でしか使ってないので関数にしなくても良さそう。

ConfigureFilter を削除して ConvertForwardingFilter にまとめます。

melpon · 2024-12-24T05:22:52Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                return JsonUtility.ToJson(ffMetadata);
+            }
+        }
+        public static Sora.ForwardingFilter CreateFilter(ForwardingFilter settings)


Create より Convert の方が機能として近そうなのと、この機能は Forwarding Filter なので略して Filter と書いてはダメです。

結果的に ForwardingFilterHelper はこの関数１個しか使わないので、ForwardingFilterHelper というクラスにまとめなくても、単に SoraSample に static 関数１個生やすだけでいいと思います。

ご指摘のとおりです。ForwardingFilterHelper にまとめるのをやめます。

melpon · 2024-12-24T05:26:50Z

SoraUnitySdkSamples/Assets/SoraSample.cs

@@ -777,30 +838,23 @@ public void OnClickStart()
            }
            fixedDataChannelLabels = config.DataChannels.Select(x => x.Label).ToArray();
        }
-        if (forwardingFilter.Length != 0)
+        if (forwardingFilter != null)


インスペクタで設定している以上、通常 forwardingFilter は null にはならないので、bool enableForwardingFilter を用意するか、以前と同じ条件にするなら forwardingFilter.rules.Length != 0 にする必要があります。

enableForwardingFilter を用意します。

melpon · 2024-12-24T05:28:36Z

SoraUnitySdkSamples/Assets/SoraSample.cs

-            foreach (var rs in forwardingFilter)
+            config.ForwardingFilter = ForwardingFilterHelper.CreateFilter(forwardingFilter);
+        }
+        if (forwardingFilters != null && forwardingFilters.Length > 0)


インスペクタで設定している以上、通常 forwardingFilters は null にはならないはずです。
あと、サンプルの仕様として長さ０の ForwardingFilters を作ることを許容するかどうか次第ですけど、こちらも bool enableForwardingFilters フラグを用意するかどうか考える必要がありそう。

何も条件がないと forwardingfilters の項目を設定してしまうのでフラグを用意します。

melpon · 2024-12-24T05:32:13Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+
+            if (settings.rules != null)
+            {
+                foreach (var ruleSet in settings.rules)


ruleSet の Set は集合だと勘違いしやすいので、別の名前を付けたほうが良いです

ruleListData にします

melpon · 2024-12-24T05:33:51Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+            {
+                foreach (var ruleSet in settings.rules)
+                {
+                    if (ruleSet?.data == null) continue;


ruleSet や ruleSet.data が null になることってあります？インスペクタで設定している以上無さそうな気がしてます。

null になることはない想定です。こちらも修正します

melpon · 2024-12-24T05:35:27Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+        {
+            public string metadata;
+        }
+        private static void ConfigureFilter(Sora.ForwardingFilter filter, ForwardingFilter settings)


settings という名前が現状と合ってなさそう

forwardingFilter に修正します

melpon · 2024-12-24T07:06:52Z

CHANGES.md

+  - マルチ転送フィルターを設定できるように項目を追加
+  - @torikizi
+- [ADD] forwardingfilter に name と priority を追加
+  - 既存の forwardingfilter に name と priority を追加


ここ [ADD] のところと同じ内容だから削って良さそう

確かに同じことが書いてあるだけでした。消します。

melpon · 2024-12-24T07:10:43Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+        if (forwardingFilter.enableName) filter.Name = forwardingFilter.name;
+        if (forwardingFilter.enablePriority) filter.Priority = forwardingFilter.priority;
+
+        if (forwardingFilter.rules != null)


インスペクタで設定される値だから forwardingFilter.rules が null になることは無いはず

ご指摘のとおりです。こちらもチェックを外します。

melpon · 2024-12-24T07:14:30Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+    {
+        public string metadata;
+    }
+    public static Sora.ForwardingFilter ConvertForwardingFilter(ForwardingFilter forwardingFilter)


sora-unity-sdk でやったのと同じように ConvertTo の方が分かりやすいかも。あと SoraSample の ForwardingFilter と Sora.ForwardingFilter が同じ名前なので、違いを出すために ConvertToSoraForwardingFilter とかにすると良さそう。

ConvertToSoraForwardingFilter にします。

melpon · 2024-12-24T07:15:37Z

SoraUnitySdkSamples/Assets/SoraSample.cs

-    public string forwardingFilterVersion = "";
-    public bool enableForwardingFilterMetadata = false;
-    public string forwardingFilterMetadataMessage = "";
+    [System.Obsolete("forwardingFilter は非推奨です。代わりに forwardingFilters を使用してください。")]


これはもう１行下で設定するのが正しそう？もしかしたら両方の変数に必要かも

対象としては両方あった方が良さそうですね。両方設定します。

melpon · 2024-12-24T07:16:11Z

SoraUnitySdkSamples/Assets/SoraSample.cs

-                var ccrs = new List<Sora.ForwardingFilter.Rule>();
-                foreach (var r in rs.data)
+                var newFilter = ConvertForwardingFilter(filter);
+                if (newFilter != null)


ここが null になることは無いはず

ご指摘のとおりです。チェックを外します。

melpon · 2024-12-24T07:44:17Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+                ruleList.Add(newRule);
+            }
+
+            if (ruleList.Any())


ruleList が空の時にも filter.Rules に空の値を設定するのが正しいので、この条件は要らないです。

条件を削除します。

melpon · 2024-12-24T07:44:53Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+
+        if (forwardingFilter.enableVersion) filter.Version = forwardingFilter.version;
+        if (forwardingFilter.enableMetadata)
+        {


ToJson １個で済むようになったので、他の if 文と同じ書き方にしておいて下さい

ご指摘のとおりです。他とあわせて一行になるようにします。

melpon · 2024-12-24T07:56:13Z

SoraUnitySdkSamples/Assets/SoraSample.cs

+        public string name;
+        public bool enablePriority = false;
+        public int priority;
+        public RuleList[] rules;


RuleList の配列だから ruleLists の方が名前的に良さそう。rules だと Rule の配列だと思ってしまいそうなので。

ruleLists に名前を変えます。

melpon

よさそう

torikizi added 2 commits December 19, 2024 16:31

マルチ転送フィルターを追加する

da028f5

変更履歴を更新

e2c73d8

torikizi changed the title ~~[WIP] マルチ転送フィルターを設定可能にする~~ マルチ転送フィルターを設定可能にする Dec 19, 2024

torikizi commented Dec 19, 2024

View reviewed changes

CHANGE ではなく ADD

3bd9be8

torikizi requested review from miosakuma and melpon December 19, 2024 07:58

転送フィルターの設定を見直し

c593ce5

melpon reviewed Dec 20, 2024

View reviewed changes

転送フィルターの処理を共通化

8521ea0

torikizi commented Dec 20, 2024

View reviewed changes

melpon reviewed Dec 21, 2024

View reviewed changes

torikizi added 4 commits December 23, 2024 01:12

さらに共通化

1c5cdd8

metadata を JSON にする場所を共通にする

91b1e92

整える

45b09ef

共通のヘルパークラスを作って設定時の処理をスリム化

53e1d6e

torikizi added 3 commits December 24, 2024 11:09

ヘルパーに全てまとめる

f12b9e0

非推奨メッセージを追加

b98f63f

Settings がついている必要はなかった

8d09916

melpon reviewed Dec 24, 2024

View reviewed changes

ヘルパーにまとめる必要はなかった

a181a49

melpon reviewed Dec 24, 2024

View reviewed changes

torikizi added 3 commits December 24, 2024 16:08

ForwardingFilter クラスの metadata は string ではない

ffd5c58

初期値いらない

c0ac41c

CHANGES の内容見直し

a670136

melpon reviewed Dec 24, 2024

View reviewed changes

Inspector で設定するケースの null チェックをやめる

d0394af

melpon reviewed Dec 24, 2024

View reviewed changes

rules ではなく ruleLists に名前を変える

7ebc714

melpon approved these changes Dec 24, 2024

View reviewed changes

マルチ転送フィルターを設定可能にする #62

Are you sure you want to change the base?

マルチ転送フィルターを設定可能にする #62

Conversation

torikizi commented Dec 19, 2024 • edited Loading

New Features:

Code Updates:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torikizi commented Dec 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torikizi Dec 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

melpon left a comment

Choose a reason for hiding this comment

torikizi commented Dec 19, 2024 •

edited

Loading

torikizi Dec 24, 2024 •

edited

Loading