Skip to content
This repository has been archived by the owner on Apr 28, 2023. It is now read-only.

TextureFX update #565

Open
wants to merge 42 commits into
base: preview/2021.4
Choose a base branch
from

Conversation

cloneproduction
Copy link

No description provided.

@tebjan
Copy link
Member

tebjan commented Jun 2, 2022

Continues #563

@tebjan
Copy link
Member

tebjan commented Jun 2, 2022

Let us know when you think that it is ready for review.

@joreg
Copy link
Member

joreg commented Sep 13, 2022

@cloneproduction first off, thanks for this massive PR.
what irritates me is that you made changes to some core files. can you please review if those were really necessary:

  • if not, please revert those changes
  • otherwise please precisely describe the changes

@cloneproduction
Copy link
Author

Hi,
sorry for the irritation. Which files are you talking about ?

@joreg
Copy link
Member

joreg commented Sep 14, 2022

you see them listed right below here when viewing this on github. it shows you the conflicting files.

@cloneproduction
Copy link
Author

Well, I believe the only VL files that I changed were :
VL.Stride.Rendering.TextureFX because some filters are Process Nodes.
VL.Stride.Rendering.ShaderFX because I added a Vector2 Join to work with ShaderFX.

No idea why the 4 files are conflicting, is there a simple way to fix this ? "Resolve conflicts" button is disabled here.

@joreg
Copy link
Member

joreg commented Sep 15, 2022

above, there is a tab labeled "Files Changed". go there and see what the changes are for these 4 files:

  • packages/VL.Stride.Runtime/VL.Stride.Engine.vl
  • packages/VL.Stride.Runtime/VL.Stride.Rendering.vl
  • packages/VL.Stride.Runtime/VL.Stride.Runtime.TypeForwards.vl
  • packages/VL.Stride.Windows/VL.Stride.Windows.vl

if you don't know what those are, you probably did them by accident. in which case you should rever the changes. otherwise please explain what they are.

@cloneproduction
Copy link
Author

VL.Stride.Engine.vl for example has this change :

From this :

<Document xmlns:p="property" Id="UFzEbZy7D3GNY8xnLUU17p" LanguageVersion="2021.4.7.892" Version="0.128">
  <NugetDependency Id="EKAeSpl0TUSO67sl9VuE19" Location="VL.CoreLib" Version="2021.4.7-0892-g77eda0271b" />

to this :

<Document xmlns:p="property" Id="UFzEbZy7D3GNY8xnLUU17p" LanguageVersion="2021.4.9.980" Version="0.128">
  <NugetDependency Id="EKAeSpl0TUSO67sl9VuE19" Location="VL.CoreLib" Version="2021.4.9-0980-g120592511d" />

I don't know why this was changed, how can I revert it ?

@cloneproduction
Copy link
Author

Just tried :

git checkout d8fcd2758ad9c23c87432dc96fa49a8fa335949f -- packages/VL.Stride.Runtime/VL.Stride.Engine.vl

But there is nothing to commit after that...

@joreg
Copy link
Member

joreg commented Sep 15, 2022

"I don't know why this was changed, how can I revert it ?"
.vl files include the version of vvvv they were last saved with. so you apparently saved that file and committed that change unwittingly. now when working with code/git it is extremely important to only commit exactly what you know needs to be changed. otherwise, things like this happen that cost a lot of time to resolve.

so i looked a bit closer now at the PRs changed files and found many more (other than those listed as conflicting) that must not be changed. it looks like at some point you simply hit "save all" and committed all changes. unfortunately, this introduces a lot of changes that make it very hard to review the essence of the PR.

i'd therefore like to suggest to start the PR again from scratch. it shouldn't really be too much work, as all the coding is already done, right? it should merely be about starting a clean branch from upstream and adding exactly specific files.

i'd also like to suggest to do this in not one large PR but rather several small ones. this should speed up review time and make it much easier for us to merge. how about we start with a PR where you only add a couple of the simpler shaders that don't need to modify any existing files. ie. only adding the bare minimum you need for the shaders. this should get you used to the workflow and we should be able to progress faster.

@joreg
Copy link
Member

joreg commented Jan 27, 2023

sorry for the delay. we had another look and here is a more specific suggestion to help us get a better overview and move forward:

  • am i assuming correctly that the changes proposed here: https://discourse.vvvv.org/t/blend-mixer-problems/20236 are part of this PR? if so: please create a new clean PR for only this change, as it seems to be quite an independent change?!
  • one big topic of this PR seems to be a change of shader inputs to Compute(). please provide another clean PR that has only those changes. as this will be a breaking change we want to take extra care that we get all those changes in, in one go and can announce them properly.
  • ideally we then get another PR that only has other types of changes/cleanups
  • and finally it should be an easy step to accept another PR with only new shaders you added

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants