Fix use_appearance_embedding for instant-ngp

[brentyi]

@liruilong940607 can you take a glance at this?

The issue I was trying to resolve was that the use_appearance_embedding config field is being ignored, which led to noticing that the TCNNInstantNGPField class isn't being used anywhere.

Perhaps this was changed temporarily to the nerfacto field and then accidentally merged?

After changing the NGP model's field from the nerfacto field to the NGP field I can still get reasonable results on lego, as long as I pass --alpha-color black to the dataparser config. (also not sure if this is the intended behavior)

cc @kevin-thankyou-lin

[brentyi]

( for context this is related to PR #1809 )

[liruilong940607]

I remember when I was working on that PR, using the nerfacto field gives noticeable better result than ngp field on the poster scene (not entirely sure now bc I didn't keep the record of it).

I was not sure what's the reason, but considering those two are very similar and the nerfacto field has been actively maintained, I felt it's a reasonable choice to just use the nerfacto field.

If you confirm the poster scene has similar result then I think it's fine to switch back. Other choice i would propose, is to modify the config use_appearance_embedding to use_average_appearance_embedding and feed it to the nerfacto field. This would keep the ngp models to have the same behavior with the PR #1809

[brentyi]

Ok I just tried it out; I can confirm that the poster scene looks worse after this PR with the default settings, but at least qualitatively the NGP and nerfacto fields look the same when I set use_appearance_embeddings=True for the NGP field. Maybe that was the cause of the discrepancy you were seeing @liruilong940607? Appearance embeddings seem important for the poster scene.

All of this does raise the question of: do we even need separate Nerfacto field and NGP field implementations? Currently the latter is completely unused, and I agree it would make sense to just modify the Nerfacto field instantiation to respect use_appearance_embedding. cc @tancik

[liruilong940607]

The only reason I was keeping the NGP field there is that the nerfplayer was relying on that (they have a ngp variation that inherits from ngp field).

I think it makes sense to just get rid of the ngp field as the nerfacto field is a superset of that, with only minor differences on the default behavior of the appearance embedding (anything else?).

[tancik]

It would be nice to remove TCNNInstantNGPField if it isn't actually useful.

[brentyi]

Sounds good, might take a few days but I can try to remove the separate NGP field implementation.

In that direction, we could also add occupancy grid support to the nerfacto model and drop the InstantNGPModel implementation too? We could still keep the instant-ngp subcommand if we want to call it that, it would just point to the same config class but with the proposal networks off by default.

[tancik]

Any updates on this @brentyi ?

[brentyi]

Forgot about it, but just added to my to-do list...

[tancik]

LGTM